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

Implement [skip ci] for Jenkins #9554

Merged
merged 2 commits into from
Jan 12, 2022
Merged

Conversation

driazati
Copy link
Member

@driazati driazati commented Nov 23, 2021

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

@driazati driazati marked this pull request as ready for review November 23, 2021 17:38
# headline

if git log --format='%s' HEAD~1..HEAD | grep --quiet \
--regexp='\[skip ci\]' \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@areusch
Copy link
Contributor

areusch commented Nov 24, 2021

couple thoughts:

  1. i think we should add unit tests for something like this so we can be sure it works correctly. it would be great to consider the cases of:
    • main has [skip ci] commit
    • PR branch has [skip ci] commit
    • no [skip ci] commit exists
  2. we might consider submitting this for RFC considering as it's a change to the development process.

i'm also curious about @leandron's comment about flagging skipped PRs.

@driazati driazati force-pushed the driazati/test_skip branch 2 times, most recently from b68189a to b980b76 Compare November 25, 2021 05:52
Jenkinsfile Outdated
script: './tests/scripts/git_skip_ci.sh',
label: "Check if CI should be skipped",
)
// if (skip_ci == 1) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these comments be removed as they are duplicated below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@driazati driazati marked this pull request as draft December 3, 2021 21:17
@driazati driazati force-pushed the driazati/test_skip branch 11 times, most recently from af98484 to 1c3b526 Compare December 9, 2021 00:50
@driazati
Copy link
Member Author

driazati commented Dec 9, 2021

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

@driazati driazati marked this pull request as ready for review December 9, 2021 20:27
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added one in 0710915

@driazati
Copy link
Member Author

Confirming sounds good, it should be ready to push now

@driazati driazati force-pushed the driazati/test_skip branch 2 times, most recently from 387183e to 1c47808 Compare December 16, 2021 00:14
@driazati driazati force-pushed the driazati/test_skip branch from 1c47808 to b48f47b Compare January 3, 2022 20:18
@areusch
Copy link
Contributor

areusch commented Jan 12, 2022

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

@areusch areusch merged commit 586944e into apache:main Jan 12, 2022
driazati added a commit to driazati/tvm that referenced this pull request Jan 12, 2022
This reverts commit 9d867c2.

Note: This PR is a test of the skip CI functionality from apache#9554. It was made with:

```bash
git checkout -b revert_test
git revert HEAD
gh pr create --draft
```
driazati added a commit to driazati/tvm that referenced this pull request Jan 13, 2022
jroesch pushed a commit that referenced this pull request Jan 13, 2022
This was inadvertently removed by #9554

Co-authored-by: driazati <[email protected]>
crazydemo pushed a commit to crazydemo/tvm that referenced this pull request Jan 27, 2022
* Rebase

* Fix missing skip

Co-authored-by: driazati <[email protected]>
crazydemo pushed a commit to crazydemo/tvm that referenced this pull request Jan 27, 2022
This was inadvertently removed by apache#9554

Co-authored-by: driazati <[email protected]>
Raghav-Chakravarthy pushed a commit to Raghav-Chakravarthy/tvm that referenced this pull request Jan 28, 2022
* Rebase

* Fix missing skip

Co-authored-by: driazati <[email protected]>
Raghav-Chakravarthy pushed a commit to Raghav-Chakravarthy/tvm that referenced this pull request Jan 28, 2022
This was inadvertently removed by apache#9554

Co-authored-by: driazati <[email protected]>
ylc pushed a commit to ylc/tvm that referenced this pull request Feb 16, 2022
* Rebase

* Fix missing skip

Co-authored-by: driazati <[email protected]>
ylc pushed a commit to ylc/tvm that referenced this pull request Feb 16, 2022
This was inadvertently removed by apache#9554

Co-authored-by: driazati <[email protected]>
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.

4 participants