Skip to content
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

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

MiguelWeezardo
Copy link
Member

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.

Is this change a fix, improvement, new feature, refactoring, or other?

This is an improvement of build stability.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

This is a change in the CI pipeline.

How would you describe this change to a non-technical end user or system administrator?

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:

# Section
* Fix some things. ({issue}`issuenumber`)

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'"
Copy link
Member

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.

@MiguelWeezardo MiguelWeezardo force-pushed the compile_all_commits branch 2 times, most recently from bafff60 to 1b6b936 Compare August 16, 2022 11:33
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@MiguelWeezardo MiguelWeezardo force-pushed the compile_all_commits branch 5 times, most recently from 484415c to 7d40c52 Compare August 17, 2022 12:22
- name: Check if all commits compile
run: |
git fetch origin
git config user.name "$(git --no-pager log --format=format:'%an' -n 1)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

@MiguelWeezardo MiguelWeezardo Aug 18, 2022

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.

Copy link
Member

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.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@MiguelWeezardo MiguelWeezardo force-pushed the compile_all_commits branch 3 times, most recently from 1f9e9e4 to 6182350 Compare August 18, 2022 08:01
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'
Copy link
Member

@hashhar hashhar Aug 18, 2022

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.

Copy link
Member

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

@MiguelWeezardo MiguelWeezardo force-pushed the compile_all_commits branch 8 times, most recently from 92d0660 to a641169 Compare August 18, 2022 21:58
@MiguelWeezardo MiguelWeezardo marked this pull request as draft August 18, 2022 21:59
@MiguelWeezardo MiguelWeezardo marked this pull request as ready for review August 18, 2022 22:38
@MiguelWeezardo MiguelWeezardo marked this pull request as draft August 19, 2022 08:37
@MiguelWeezardo MiguelWeezardo force-pushed the compile_all_commits branch 2 times, most recently from 8fe57f0 to a641169 Compare August 19, 2022 09:58
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % 1 nit

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@hashhar
Copy link
Member

hashhar commented Oct 10, 2022

Do you have some CI runs with this within your fork? master behaves a bit differently with GIB so we can't verify it before it's merged otherwise.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@MiguelWeezardo MiguelWeezardo force-pushed the compile_all_commits branch 2 times, most recently from 0b5562d to f1e029e Compare October 10, 2022 11:22
@MiguelWeezardo
Copy link
Member Author

Do you have some CI runs with this within your fork? master behaves a bit differently with GIB so we can't verify it before it's merged otherwise

I've pushed changes in 3 commits so we can see it in action. We can squash everything before merge.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % logging improvements

@hashhar
Copy link
Member

hashhar commented Oct 11, 2022

(remember to squash before merge)

docs/build Outdated Show resolved Hide resolved
@hashhar
Copy link
Member

hashhar commented Oct 14, 2022

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?

@MiguelWeezardo
Copy link
Member Author

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 rebase --exec you have twice as many rebase steps than commits, because the rebase log looks like this:

pick X commit 1
exec cmd
pick Y commit 2
exec cmd

Compilation happens on even-numbered steps.

@hashhar
Copy link
Member

hashhar commented Oct 14, 2022

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 --versbose it's less confusing since it says which commit has been checked out.

@hashhar
Copy link
Member

hashhar commented Oct 14, 2022

@MiguelWeezardo Please squash after you have a green run with the latest changes. It's good to be merged now.

@MiguelWeezardo
Copy link
Member Author

@hashhar I think we're all set. Could you merge?

@hashhar
Copy link
Member

hashhar commented Oct 17, 2022

@MiguelWeezardo Sorry for one more comment. Do you think it's be useful to print git log --graph --reverse instead of in forward direction? That way the commits on this branch would be listed at top of output instead after long list of commits on master?

@hashhar hashhar merged commit bd46101 into trinodb:master Oct 18, 2022
@github-actions github-actions bot added this to the 401 milestone Oct 18, 2022
@MiguelWeezardo MiguelWeezardo deleted the compile_all_commits branch October 18, 2022 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed docs no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

6 participants