.github: Skip unnecessary workflow steps #16157
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pchaigno
added
area/CI-improvement
pchaigno
force-pushed
the
skip-unecessary-workflow-steps
branch
12 times, most recently
from
905f108
to
80824e0
Compare
pchaigno
force-pushed
the
skip-unecessary-workflow-steps
branch
from
80824e0
to
3e56cc4
Compare
pchaigno
commented
May 18, 2021
| @@ -19,11 +19,42 @@ env: | |||
| check_url: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }} | |||
|
|
|||
| jobs: | |||
| check_changes: | |||
| name: Deduce required tests from code changes | |||
There was a problem hiding this comment.
For reviewers: This job could also be integrated into installation-and-connectivity to reduce the number of different jobs displayed for each pull request. The downside of that approach is that the job would not be clearly marked as skipped (since only a subset of steps would be skipped).
pchaigno
commented
May 18, 2021
aanm
reviewed
May 18, 2021
pchaigno
force-pushed
the
skip-unecessary-workflow-steps
branch
from
3e56cc4
to
bd52cdc
Compare
aanm
approved these changes
May 19, 2021
pchaigno
force-pushed
the
skip-unecessary-workflow-steps
branch
from
69499d6
to
9981ad9
Compare
nbusseneau
requested changes
May 19, 2021
There was a problem hiding this comment.
Nice job, I had this in my list as well but I only thought about doing it at the workflow-level (see my comment below), not at the job or steps-level.
I actually wonder if it might be worth to refactor/split some of the actions here in independent workflows so that we take advantage of the native workflow-level filtering, which is more efficient than the dorny/paths-filter one.
If a pull request doesn't touch the bpf/ directory, we don't need to check that the datapath compiles for every commit. Signed-off-by: Paul Chaignon <paul@cilium.io>
The coccinelle and build checks are unnecessary when files under bpf/ and contrib/coccinelle/ are untouched. We still need to run checkpatch because it actually verifies all commits and not only code touching bpf/. Signed-off-by: Paul Chaignon <paul@cilium.io>
We don't need to run documentation tests if the documentation is untouched. Signed-off-by: Paul Chaignon <paul@cilium.io>
We don't need to run the smoke tests if only Documentation/ or test/ are touched. We therefore skip them in such case. Obviously, we could extend this beyond these two directories, but the goal here is to keep the list simple while catching the most common cases. Signed-off-by: Paul Chaignon <paul@cilium.io>
We don't need to run the ci-xxx end-to-end tests if only Documentation/ or test/ are touched. We therefore skip them in case of the trigger phrase 'test-me-please'. If these tests are explicitly requested via the 'ci-xxx' trigger phrase, we run them. Obviously, we could extend this beyond these two directories, but the goal here is to keep the list simple while catching the most common cases. Signed-off-by: Paul Chaignon <paul@cilium.io>
pchaigno
force-pushed
the
skip-unecessary-workflow-steps
branch
from
9981ad9
to
6b98ab5
Compare
pchaigno
added
the
ready-to-merge
pchaigno
added a commit
to pchaigno/cilium
that referenced
this pull request
Jun 9, 2021
In its initial version, commit 3ceb742 (".github: Skip unnecessary docs test") skipped the job instead of the whole workflow, based on changed code paths. After discussion on the pull request [1], we figured we could save some seconds by skipping the whole workflow instead of the job. Skipping the job requires adding a new job to check the code paths. However, if we skip the whole workflow, the job status is never reported on GitHub. Since the documentation job is required to merge, that means maintainer's little helper will never mark the pull request as ready to merge. Thus, this commit reverts back to skipping the jobs instead of the whole workflows. To that end, we use paths-filter GitHub Action instead of the direct GitHub integration (since the GitHub path filters can only skip the whole workflow). 1 - cilium#16157 (comment) Signed-off-by: Paul Chaignon <paul@cilium.io>
aanm
pushed a commit
that referenced
this pull request
Jun 10, 2021
In its initial version, commit 3ceb742 (".github: Skip unnecessary docs test") skipped the job instead of the whole workflow, based on changed code paths. After discussion on the pull request [1], we figured we could save some seconds by skipping the whole workflow instead of the job. Skipping the job requires adding a new job to check the code paths. However, if we skip the whole workflow, the job status is never reported on GitHub. Since the documentation job is required to merge, that means maintainer's little helper will never mark the pull request as ready to merge. Thus, this commit reverts back to skipping the jobs instead of the whole workflows. To that end, we use paths-filter GitHub Action instead of the direct GitHub integration (since the GitHub path filters can only skip the whole workflow). 1 - #16157 (comment) Signed-off-by: Paul Chaignon <paul@cilium.io>
nathanjsweet
pushed a commit
that referenced
this pull request
Jun 16, 2021
[ upstream commit b08f700 ] In its initial version, commit 3ceb742 (".github: Skip unnecessary docs test") skipped the job instead of the whole workflow, based on changed code paths. After discussion on the pull request [1], we figured we could save some seconds by skipping the whole workflow instead of the job. Skipping the job requires adding a new job to check the code paths. However, if we skip the whole workflow, the job status is never reported on GitHub. Since the documentation job is required to merge, that means maintainer's little helper will never mark the pull request as ready to merge. Thus, this commit reverts back to skipping the jobs instead of the whole workflows. To that end, we use paths-filter GitHub Action instead of the direct GitHub integration (since the GitHub path filters can only skip the whole workflow). 1 - #16157 (comment) Signed-off-by: Paul Chaignon <paul@cilium.io> Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
christarazi
pushed a commit
that referenced
this pull request
Jun 22, 2021
[ upstream commit b08f700 ] In its initial version, commit 3ceb742 (".github: Skip unnecessary docs test") skipped the job instead of the whole workflow, based on changed code paths. After discussion on the pull request [1], we figured we could save some seconds by skipping the whole workflow instead of the job. Skipping the job requires adding a new job to check the code paths. However, if we skip the whole workflow, the job status is never reported on GitHub. Since the documentation job is required to merge, that means maintainer's little helper will never mark the pull request as ready to merge. Thus, this commit reverts back to skipping the jobs instead of the whole workflows. To that end, we use paths-filter GitHub Action instead of the direct GitHub integration (since the GitHub path filters can only skip the whole workflow). 1 - #16157 (comment) Signed-off-by: Paul Chaignon <paul@cilium.io> Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We increasingly rely on GitHub actions to run tests on pull requests and branches, including long end-to-end tests and per-commit builds. We are however limited to 20 concurrent workflow jobs, with excess jobs being queued. This recently led to issues where long waiting queues can delay releases and pull requests.
This pull request proposes to use the dorny/paths-filter GitHub action to filter unnecessary workflow steps and jobs. This will in particular allow us to skip unnecessary tests when the documentation or the BPF code are not touched by a pull request. It also enables skipping GitHub-based end-to-end tests when only ginkgo-based tests are touched.
For most workflows changed by this pull request, the effect can be seen in the pull request's action. However, workflows triggered by issue comments run the workflow version from
master. To allow you to see this in action for such workflows, I have merged this pull request in my fork and created three test cases:Documentation/-only change.test/-only change.I'm happy to create more test cases if needed.