Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: skip jobs for chore commits #692

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: skip jobs for chore commits #692

wants to merge 4 commits into from

Conversation

PeSme
Copy link
Contributor

@PeSme PeSme commented Nov 10, 2023

Please read CONTRIBUTING.md for additional information on contributing to this repository!

What this PR does / why we need it

We have recently done a lot of commits just to update docs or terraform. And we usually use chore: prefix for those commits as we do not need a release. There are no code changes (and it's on the PR reviewer to make sure chore commits do not change any code) but still all the long running builds are executed. It takes time, costs credits and does not bring any values. This PR introduces a step skip_on_chore_commit which can be used with various jobs we would like to skip.

This is how an equivalent was developed and tested:
https://github.com/getoutreach/orexservice/pull/632/files

This is how it looks like in CircleCIL
https://app.circleci.com/pipelines/github/getoutreach/orexservice/3563/workflows/f4e52475-24f3-4d4e-aa6e-b8e97510d321

Jira ID

[XX-XX]

Notes for your reviewers

@PeSme PeSme requested a review from a team as a code owner November 10, 2023 23:02
@malept
Copy link
Member

malept commented Nov 10, 2023

CircleCI has built-in functionality for this: https://circleci.com/docs/skip-build/

@PeSme
Copy link
Contributor Author

PeSme commented Nov 10, 2023

CircleCI has built-in functionality

Well, it's not the same. You would need to add [skip ci] to every commit of the branch, not just the first one. And more importantly it would allow you introduce badly formatted files which would fail in linter validation later. I really do want to keep linters and unit tests.

@malept
Copy link
Member

malept commented Nov 10, 2023

I am generally worried that this is pretty arbitrary and not necessarily intuitive as to which jobs get "skipped" for a chore, particularly for newly onboarded engineers. Why run unit tests for chore if it's not touching code, but not run E2E tests?

Also, what about PRs which start with docs:?

@jaredallard
Copy link
Contributor

I agree with @malept here, if a developer wishes to skip CI, they should use the [skip ci] notation that is used by most CI systems. I also think skipping CI for any commits is likely to cause more issues than it solves because likely developers will accidentally include changes there that could cause breaks.

@PeSme
Copy link
Contributor Author

PeSme commented Nov 10, 2023

Would be great to have make linters.

pretty arbitrary and not necessarily intuitive

Well, why do we skip deployment for chore? Which other commit types have this effect?
There used to be a link to https://outreach-io.atlassian.net/wiki/spaces/EN/pages/1902444645/Conventional+Commits#No-Release in the initial PR message. We can describe it here. Other reasonable options can be added, of course. I would choose ci, docs, chore probably.

Hmm, it reminds me I should exclude scope from the message.

Alternative approach would be to define parameters for the command, one for each type. And add custom blocks for every job in stencil. So the conditions can be fully configured per repo/use case like this https://github.com/getoutreach/orexservice/pull/632/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R388

      pre-steps:
        - local/skip_on_chore_commit

@getoutreach-ci-1
Copy link
Contributor

Link to code coverage report (posted by coverbot 🤖)

@getoutreach-ci-2
Copy link

Link to code coverage report (posted by coverbot 🤖)

@getoutreach-ci-1
Copy link
Contributor

Link to code coverage report (posted by coverbot 🤖)

@PeSme
Copy link
Contributor Author

PeSme commented Dec 5, 2023

@jaredallard , @malept I still would like to get this merged. I did several changes based on our conversation. Is there anything else you would like to see?

@getoutreach-ci-1
Copy link
Contributor

Link to code coverage report (posted by coverbot 🤖)

@malept
Copy link
Member

malept commented Dec 5, 2023

Ultimately, what I want to see is unsurprising behavior when it comes to skipping CI. I want to avoid increased support requests of the type "why isn't my PR passing CI?" or "why aren't my required checks passing when it's a chore?"

I believe that having certain prefixes skip CI for an entire sets of conventional commit prefixes is a surprising behavior, especially for newly onboarded engineers. There may also be a unintended side effect where certain required checks on a repository will cause PRs to be unmergeable since the jobs aren't run.

Additionally, I disagree with having ci: skip certain jobs, otherwise CI changes won't get tested.

My proposal is to have arbitrary commits in non-default branches explicitly have [skip tests] at the end of the commit message. Commits with that constraint in a pull request can skip test/e2e/Node.js test jobs. But every commit on the default branch should be tested by CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants