If the HEAD commit message of a pull request contains specific keywords, it is skipped by Cirrus and/or Github.
Have the “test each commit” job do so as well.
There’s a handful of historical commits that use either [ci skip] or [skip ci]:
0git log | grep '\[ci skip]\|\[skip ci]'
Including at least one that was part of a pull request: #30898
That said, it’s fairly rare.
If the HEAD commit message of a pull request contains specific
keywords, it is skipped by Cirrus and/or Github.
Have the "test each commit" job do so as well.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | jonatack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
ci skip or not into a commit. Also, it would be distracting to review. Also, it would be distracting to have this permanently in the commit log. Also, it seems like premature optimization and was never used in the last 8 years, according to git log --grep='ci skip' -1.
This should be discouraged, but I don’t think it should be impossible.
This happens already today, with no further changes needed. The CI task will turn red to discourage it, but it is not impossible to review or merge the pull request.
1 commit every 4 years, or even 1 commit every year that qualifies for this just doesn’t seem worth it the overhead.
If you want to break git bisect, there should be a good reason. A reason well enough that reviewers run the appropriate git rebase --interactive --exec locally. If there is no good enough reason, then the breaking change probably shouldn’t be done.
A reason well enough that reviewers run the appropriate git
rebase --interactive --execlocally.
That seems a fairly arbitrary metric for measuring how good a reason is.
I’m more sympathetic to the possibility that both the reviewers and maintainer overlook a [skip ci] commit message (especially if it’s not on the first line), causing git bisect to break. And worse, potentially allowing to sneak in a non-refactoring change.
This brings back memories when contributing to Ruby on Rails, that skip ci was requested to be used in any doc-only changes.
No strong opinion, but when opening or updating a PR that only touches a markdown file, I’d use this.
when opening or updating a PR that only touches a markdown file, I’d use this
We might still want to run spell check and such things for documentation changes. Skipping unnecessary tasks could be done by CI itself. So I don’t think that’s an argument for this PR.
when opening or updating a PR that only touches a markdown file, I’d use this
We might still want to run spell check and such things for documentation changes.
Insofar as the spelling linter in the CI doesn’t raise, running it in the CI isn’t very important (and it could probably be removed) – it’s a facultative aid that can be run locally by authors and reviewers. Thus, I’d mildly favor using skip ci on my own pulls to avoid wasting CI cycles where they aren’t needed.
Concept ACK.
when opening or updating a PR that only touches a markdown file, I’d use this
We might still want to run spell check and such things for documentation changes.
Insofar as the spelling linter in the CI doesn’t raise
There are many other linters that do raise, and even many that only serve to lint docs. If they serve no purpose, they should be removed, instead of being (required to be) skipped.
Regardless, many doc updates are only checked at compile time (doxygen, or RPC docs). So skipping CI on them may lead to missed compile failures.
Overall, I think when adding a new feature (even if it is “just” CI code), there should be a reason and motivation, along with some evidence that it will be used correctly and meaningfully.
Looking at the comments so far, it looks like the motivation is weak at best, there doesn’t seem to be any evidence of a (useful) use-case in any past commit, and it could even promote incorrect usage, leading to issues down the line.
If I understand correctly, reviewers (or possibly DrahtBot) should signal if [skip ci] is being used inappropriately in any case.
If skipping it is considered appropriate for certain changes (e.g. markdown files as mentioned above) to save time and compute resources, then this pull makes sense to me.
If skipping it is discouraged in all cases, then yes, no need for this update – in which case it may be useful for the bot, if feasible, to signal any case it detects that [skip ci] is used.
If skipping it is considered appropriate for certain changes (e.g. markdown files as mentioned above) to save time and compute resources, then this pull makes sense to me.
I don’t think it is appropriate to skip, even for markdown, see the reply above: #30898 (comment)
If skipping it is discouraged in all cases, then yes, no need for this update – in which case it may be useful for the bot, if feasible, to signal any case it detects that
[skip ci]is used.
Yeah, that could make sense. However it hasn’t been used for years, so writing any code probably isn’t worth it.
I don’t think it is appropriate to skip, even for markdown, see the reply above: #30898 (comment)
There are many other linters that do raise, and even many that only serve to lint docs. If they serve no purpose, they should be removed, instead of being (required to be) skipped.
Which linters only serve to lint documentation-only files? (I could be missing some.)
Regardless, many doc updates are only checked at compile time (doxygen, or RPC docs). So skipping CI on them may lead to missed compile failures.
I don’t see a suggestion in the previous conversation to skip these.
I don’t see a suggestion in the previous conversation to skip these.
All but a few linters apply to all files (this includes doc-only, or markdown-only files). See for example the subtree check: https://github.com/bitcoin/bitcoin/runs/30239893248
0src/crc32c in HEAD currently refers to tree 81b971cac88189a11a3d7491d8e57437f534d195
1src/crc32c in HEAD was last updated in commit 5d45552fd4303f8d668ffbc50cce1053485aeead (tree 454691a9b89ee8b9e1f71a48a7398edba49c3805)
2diff --git a/README.md b/README.md
3index 58ba38e611..9ed583c213 100644
4--- a/README.md
5+++ b/README.md
6@@ -85,7 +85,8 @@ cmake -DCRC32C_BUILD_TESTS=0 -DCRC32C_BUILD_BENCHMARKS=0 .. && make all install
7
8 ## Development
9
10-The following command (when executed from `build/`) (re)builds the project and
11+The following command (when executed from within your build directory,
12+for example,`build/`) (re)builds the project and
13 runs the tests.
14
15 ```bash
16FAIL: subtree directory was touched without subtree merge
(Similar examples can be constructed for most other linters)
Which linters only serve to lint documentation-only files? (I could be missing some.)
I don’t think it would be useful to create a list of linters for doc-only changes and skip the others. All of this just seems like massive over-engineering to possibly save a millisecond or two in a CI run every 4 years.
The only use case I have in mind is a large refactor where, e.g. to make the diff easier to follow, an intermediate commit can’t build.
I’m not seeing much excitement for that functionality. Such PR can always cherry-pick this commit, so closing for now.