cirrus.yml: automatic skips based on source #23174
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
We were not able to find or create Copr project Please check your configuration for:
|
|
TODO: fix the docs in CIModes.md and test these changes in separate PRs |
| # Docs: ./contrib/cirrus/CIModes.md | ||
| only_if: ¬_tag_branch_build_docs_machine >- | ||
| $CIRRUS_PR != '' && |
There was a problem hiding this comment.
This is what I don't understand. Docker-py, apiv2, compose tests were all skipped in cron runs?
@cevich why?
I changed it so they run because I see no reason why they should not
There was a problem hiding this comment.
IIRC, we use to skip nearly everything except build and system tests. I'm not opposed to re-adding everything back in given that we're now extensively skipping in PRs.
contrib/cirrus/cirrus_yaml_test.py
Outdated
| @@ -60,20 +60,20 @@ def test_depends(self): | |||
| msg=('No success aggregation task depends_on "{0}"'.format(task_name)) | |||
| self.assertIn(task_name, success_deps, msg=msg) | |||
|
|
|||
| def test_skips(self): | |||
| def test_only_if(self): | |||
| """2024-06 PR#23030: ugly but necessary duplication in skip conditions. Prevent typos or unwanted changes.""" | |||
There was a problem hiding this comment.
not to self update PR number to this one for next push
|
Note I pushed a extra commit in case somone already looked at the existing changes, I will squash them once this is ready. |
|
unrelated to the PR but two f39 int timeouts look concerning |
|
Both timeouts happen shortly after ~500s which seems odd. Yesterday I saw a bunch of (what looked like) Cirrus provisioning errors. I wonder if these are similar, or perhaps fallout from those? |
| ### Intended general PR Tasks (*italic*: matrix) | ||
| + *build* | ||
| + validate | ||
| + bindings |
There was a problem hiding this comment.
I have removed these parts because I find them very strange they just duplicate knowledge from the cirrus.yml without explanation and this is out of date and was not updated anyway
There was a problem hiding this comment.
Some comments say "source code is changed", some say "source code is changed AND NOT only test files". This suggests there's a difference in behavior, but AFAICT there isn't. Could you pick one of those for all? Or maybe "actual source code", "non-test source code changed", or something like that?
This is really good work.
contrib/cirrus/cirrus_yaml_test.py
Outdated
| @@ -60,20 +60,20 @@ def test_depends(self): | |||
| msg=('No success aggregation task depends_on "{0}"'.format(task_name)) | |||
| self.assertIn(task_name, success_deps, msg=msg) | |||
|
|
|||
| def test_skips(self): | |||
| def test_only_if(self): | |||
| """2024-06 PR#23030: ugly but necessary duplication in skip conditions. Prevent typos or unwanted changes.""" | |||
|
FYI I've added a TODO item to reexamine https://github.com/containers/podman/blob/main/contrib/cirrus/pr-should-include-tests and find a solution that does not involve |
Idea: move the pr-should-include-tests into its own task and then have a condition to only run this task when source files are changed. |
As we want to get rid of the special titles convert the existing skips to the only_if condition, this makes it more readable as we do not need to negate so much. Then add similar conditions for all test tasks, this removes the need to a special title such as CI:DOCS as the logic is smart enough to only docs changes when no source code was changed. Update the documentation for the new logic and no longer point contributors to the CI:DOCS title as it is gone now. There is a bunch of duplication in the rules as yaml doesn't allow us to share only parts of a string. To prevent unwanted drift a test case in contrib/cirrus/cirrus_yaml_test.py is added to ensure all conditions follow the same base ruleset. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
/lgtm |
0358325
into
containers:main
Background: As of containers#23174, CI is selectively run depending on the set of files changed. Changes to version.go should require a full run of all tests, because we want releases tested. Problem: RC releases bump version.go and then bump it back. Cirrus's changesInclude() sees this as a NOP, so it does not trigger the "run all CI tests" rule. Solution: require CI:ALL in release PRs. Add step to process. Signed-off-by: Ed Santiago <santiago@redhat.com>
As we want to get rid of the special titles convert the existing skips to the only_if condition, this makes it more readable as we do not need to negate so much.
Then add similar conditions for all test tasks, this removes the need to a special title such as CI:DOCS as the logic is smart enough to only docs changes when no source code was changed.
Does this PR introduce a user-facing change?