-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Compile all commits of a PR branch #13688
Conversation
7524db4
to
9464c54
Compare
.github/workflows/ci.yml
Outdated
run: | | ||
export MAVEN_OPTS="${MAVEN_INSTALL_OPTS}" | ||
$RETRY $MAVEN clean verify -B --strict-checksums -V -T C1 -DskipTests -P ci -pl '!:trino-server-rpm' | ||
git rebase master -x "$RETRY $MAVEN clean verify -B --strict-checksums -V -T C1 -DskipTests -P ci -pl '!:trino-server-rpm'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show an example of failure for a non-head commit? Would it be easy enough to identify which commit failed?
Also, I don't think we want to run all checks on all commits except the head. We only want to make sure other commits compile.
bafff60
to
1b6b936
Compare
484415c
to
7d40c52
Compare
.github/workflows/ci.yml
Outdated
- name: Check if all commits compile | ||
run: | | ||
git fetch origin | ||
git config user.name "$(git --no-pager log --format=format:'%an' -n 1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Git rebase needs an identity to assign to commits. I'm using the identity from the top commit in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide any random identity - doesn't matter since we won't use the results.
1f9e9e4
to
6182350
Compare
.github/workflows/ci.yml
Outdated
git fetch origin | ||
git config user.name "$(git --no-pager log --format=format:'%an' -n 1)" | ||
git config user.email "$(git --no-pager log --format=format:'%ae' -n 1)" | ||
git rebase origin/master -x '$MAVEN compile -DskipTests' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebasing on origin/master instead of PR head means that the commit hashes for the rebased result may change (since other PRs could've been merged in meantime) and users will have harder time to figure out which commit caused compilation failure.
Also not every PR is open against master - they can target different branches as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are examples elsewhere in this workflow how to get current PRs head
92d0660
to
a641169
Compare
8fe57f0
to
a641169
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % 1 nit
Do you have some CI runs with this within your fork? |
0b5562d
to
f1e029e
Compare
I've pushed changes in 3 commits so we can see it in action. We can squash everything before merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % logging improvements
(remember to squash before merge) |
3425c25
to
d61e7c3
Compare
One this which verbose is showing is that it seems we are running compilation for alternate commits only? i.e. I see maven log output only for some steps of the rebase. Is there some way to verify it is indeed running compilation for each commit? |
With
Compilation happens on even-numbered steps. |
Ah, that clears it up for me. I did not realise the numbers are for each "action" instead of commit - it makes total sense though why it looks like it does. Thankfully with |
@MiguelWeezardo Please squash after you have a green run with the latest changes. It's good to be merged now. |
d61e7c3
to
03ec740
Compare
@hashhar I think we're all set. Could you merge? |
@MiguelWeezardo Sorry for one more comment. Do you think it's be useful to print |
03ec740
to
4deb38f
Compare
4deb38f
to
a13c07b
Compare
Description
To make automated git bisect work better, we need to make sure all commits in a PR are at least compiling.
This change calls Maven install for all commits between master and the PR branch.
This is an improvement of build stability.
This is a change in the CI pipeline.
Related issues, pull requests, and links
Documentation
(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
(x) No release notes entries required.
( ) Release notes entries required with the following suggested text: