Run compilation check as a composite action#15157
Conversation
MiguelWeezardo
force-pushed
the
check-commits-composite-action
branch
2 times, most recently
from
ca77d75 to
2801898
Compare
MiguelWeezardo
force-pushed
the
check-commits-composite-action
branch
5 times, most recently
from
baa350d to
e1b8e4e
Compare
There was a problem hiding this comment.
why not fail-fast? If some intermediate commit is broken by definition someone needs to fix the commit hence invalidating all results of any commits that come after the broken one?
There was a problem hiding this comment.
Please undo this change - my understanding was wrong.
The matrix entries are only created in a specific order - their execution order can be different/concurrent unless max-parallel: 1 is added.
Each check-commit takes ~10mins at most so doing it serially via max-parallel: 1 doesn't sound good.
So let's disable fail-fast like it was before.
There was a problem hiding this comment.
This was supposed to be a simple pass/fail check, we don't have to find all broken commits if we already know the check will fail.
There was a problem hiding this comment.
that's a fair point - we can offload which commit is broken to PR author.
MiguelWeezardo
force-pushed
the
check-commits-composite-action
branch
4 times, most recently
from
8676b9b to
8de10a2
Compare
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
LGTM % can you add a temporary 2nd commit to this PR which changes ci.yml in some way?
I already tested this using my own fork: MiguelWeezardo#12 |
In our CI pipeline we want to test if all commits of a PR would compile if checked out individually. This ensures commits don't have dependencies on changes added afterwards. This makes git bisect automation possible. Previous approach used a step in the main CI workflow, which is loaded when a CI workflow starts. This means that the method of compiling each commit in the entire PR branch will be the one used for the PR HEAD. This may cause problems if the PR changes any Maven parameters in any commit except the first one. Consider for example adding a settings.xml file to Maven parameters in a second commit of a PR. Since the environment variables from PR HEAD are in effect for the entire workflow run, the workflow would call Maven with the `-s settings.xml` option for all commits of the PR. The commit adding the file might be self-contained, but if it's not the first commit of the PR, it will trigger a failure for older commits.
MiguelWeezardo
force-pushed
the
check-commits-composite-action
branch
from
a864e85 to
b798d6f
Compare
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
LGTM % undo testing changes + #15157 (comment)
MiguelWeezardo
force-pushed
the
check-commits-composite-action
branch
from
b798d6f to
eae9f6e
Compare
hashhar
added
the
no-release-notes
3 participants
Description
Previous compilation check had a design flaw.
It used environment definitions from the HEAD of the PR for all commits in the PR.
Additional context and related issues
In our CI pipeline we want to test if all commits of a PR would compile if checked out individually.
This ensures commits don't have dependencies on changes added afterwards.
This makes it possible to automate git bisect.
Previous approach used a step in the main CI workflow, which is loaded when a CI workflow starts.
This means that the method of compiling each commit in the entire PR branch will be the one used for the PR HEAD.
This may cause problems if the PR changes any Maven parameters in any commit except the first one.
Consider for example adding a
settings.xmlfile to Maven parameters in a second commit of a PR.Since the environment variables from PR HEAD are in effect, the CI workflow would call Maven with the
-s settings.xmloption for all commits of the PR. The commit adding the file might be self-contained, but if it's not the first commit of the PR, it will trigger a failure for older commits.Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: