Implement [skip ci] for Jenkins#9554
Implement [skip ci] for Jenkins#9554areusch merged 2 commits intoapache:mainfrom driazati:driazati/test_skip
Conversation
driazati
requested review from
a team,
areusch,
comaniac,
icemelon,
jroesch,
junrushao,
merrymercy,
tqchen and
yzhliu
as code owners
tests/scripts/git_skip_ci.sh
Outdated
| # headline | ||
|
|
||
| if git log --format='%s' HEAD~1..HEAD | grep --quiet \ | ||
| --regexp='\[skip ci\]' \ |
There was a problem hiding this comment.
As this is something new we are introducing, we could start with only one documented pattern rather than all these option variants, which makes a bit harder to manually grep on which commits in history didn’t run a full CI.
I am also a bit concerned on how we make sure that only exceptional changes that don’t really need CI will make use of this. As an extension, I wonder if Jenkins could somehow tag the PR with something when it detects the “skip ci” tag, so that it is obvious for reviewers that this didn’t run any checks, apart from lint.
Any idea on how to tackle these?
There was a problem hiding this comment.
Agreed on consolidating on a single trigger message like [skip ci] for now. At some level we'd need to rely on committers / mergers knowing that [skip ci] should only be applied to quick fixes and reverts to unbreak master which seems do-able. We could make it easier to tell without clicking by add a label like CI was skipped to the PR (through GitHub Actions or (ideally) directly from Jenkins)
There was a problem hiding this comment.
Agreed. To me, that is a must to have this PR in.
Another point, just to mention, is that we ideally would like to document it somewhere, so that it is clear for the community this is available and how to use it.
|
couple thoughts:
i'm also curious about @leandron's comment about flagging skipped PRs. |
Jenkinsfile
Outdated
| script: './tests/scripts/git_skip_ci.sh', | ||
| label: "Check if CI should be skipped", | ||
| ) | ||
| // if (skip_ci == 1) { |
There was a problem hiding this comment.
Should these comments be removed as they are duplicated below?
There was a problem hiding this comment.
sorry this was some work-in-progress code, I should have marked this as a draft since I'm still working on implementing the PR labeling aspect
|
There are some test usages here showing how skip is triggered (i.e. the CI on the first commit in that PR takes only a few minutes): #9686 |
areusch
left a comment
areusch
left a comment
There was a problem hiding this comment.
one question here--also, we should push this to ci-docker-staging one final time i think. just checking that makes sense to you?
| ["commit", "--allow-empty", "--message", "[skip ci] commit 1"], | ||
| ], | ||
| should_skip=True, | ||
| why="ci should be skipped on a branch with [skip ci] in the last commit", |
There was a problem hiding this comment.
do we have a test for when [skip ci] is in a commit but not in PR title? i don't think we should allow only the commit to have [skip ci]
|
Confirming sounds good, it should be ready to push now |
|
looks like the branch failed due to a flaky test. given passing tests linked from #9686, i think we can merge and be confident we aren't going to break the Jenkinsfile |
This was inadvertently removed by #9554 Co-authored-by: driazati <driazati@users.noreply.github.com>
* Rebase * Fix missing skip Co-authored-by: driazati <driazati@users.noreply.github.com>
This was inadvertently removed by apache#9554 Co-authored-by: driazati <driazati@users.noreply.github.com>
* Rebase * Fix missing skip Co-authored-by: driazati <driazati@users.noreply.github.com>
This was inadvertently removed by apache#9554 Co-authored-by: driazati <driazati@users.noreply.github.com>
* Rebase * Fix missing skip Co-authored-by: driazati <driazati@users.noreply.github.com>
This was inadvertently removed by apache#9554 Co-authored-by: driazati <driazati@users.noreply.github.com>
4 participants
GitHub Actions already respects this (link), this implements it for Jenkins as well. When
[skip ci]is in the commit message of the head commit and in the PR title, the only things that will run are the sanity check + lint, so CI should be much quicker if someone manually sets this.This is useful for PRs that need to get landed quickly and the outcome is pretty well known, such as reverts (and should only be used in these situations). Tested in #9686
Also see the related forum discussion: https://discuss.tvm.apache.org/t/rfc-ci-add-a-skip-ci-tag-to-shortcut-builds-and-tests/11589/10
@areusch