Skip to content

Run check-commits dispatcher only if PR branch has compile-commit job#15210

Merged
hashhar merged 2 commits intotrinodb:masterfrom
MiguelWeezardo:check-commits-fix-action-not-found
Dec 22, 2022
Merged

Run check-commits dispatcher only if PR branch has compile-commit job#15210
hashhar merged 2 commits intotrinodb:masterfrom
MiguelWeezardo:check-commits-fix-action-not-found

Conversation

@MiguelWeezardo
Copy link
Copy Markdown
Member

@MiguelWeezardo MiguelWeezardo commented Nov 28, 2022

Description

#15157 introduced a composite job for compiling every commit of a PR.

Before running check-commits dispatcher it's important to check that the PR branch contains the definition of the compile-commit composite job.

A bug was also discovered in the job responsible for selecting commits to be compiled.
The rev-list query by default returns the list of commit hashes in reverse chronological order.
The last rev-list entry removed by the jq pipeline expression was the oldest PR commit, not the newest one.

This commit fixes the problem by using the rev-list expression instead of jq pipeline to filter out PR HEAD.

Additional context and related issues

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:

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

@cla-bot cla-bot Bot added the cla-signed label Nov 28, 2022
@MiguelWeezardo
Copy link
Copy Markdown
Member Author

Tested with MiguelWeezardo#12

@findepi findepi removed their request for review November 28, 2022 16:55
@MiguelWeezardo MiguelWeezardo force-pushed the check-commits-fix-action-not-found branch from 140e7ed to 7c3dc6e Compare November 29, 2022 12:39
@MiguelWeezardo MiguelWeezardo changed the title Fix check-commits for PRs opened before it was merged Fix commit selection in check-commits Nov 29, 2022
Comment thread .github/workflows/ci.yml Outdated
@MiguelWeezardo
Copy link
Copy Markdown
Member Author

MiguelWeezardo commented Nov 29, 2022

During my investigation I have found a bug in the commit selection pipeline which I intend to fix here as well.

@MiguelWeezardo MiguelWeezardo force-pushed the check-commits-fix-action-not-found branch 2 times, most recently from abae95a to cebaac0 Compare November 29, 2022 13:00
@MiguelWeezardo MiguelWeezardo marked this pull request as draft November 29, 2022 14:34
A bug was discovered in the pipeline responsible for
selecting commits to be compiled. The `rev-list` query returned
the list of commit hashes in **reverse** chronological order.

The last `rev-list` entry removed by the jq pipeline expression
was the oldest PR commit, not the newest one.

This commit fixes the problem by using the `rev-list` expression
instead of `jq` pipeline to filter out PR HEAD
@MiguelWeezardo MiguelWeezardo force-pushed the check-commits-fix-action-not-found branch from cebaac0 to 30065f4 Compare November 29, 2022 20:22
Before running check-commits dispatcher it's important to check that
the PR branch contains the definition of the `compile-commit` composite job.
@MiguelWeezardo MiguelWeezardo changed the title Fix commit selection in check-commits Run check-commits dispatcher only if PR branch has compile-commit job Nov 29, 2022
@MiguelWeezardo MiguelWeezardo marked this pull request as ready for review November 29, 2022 22:08
@MiguelWeezardo
Copy link
Copy Markdown
Member Author

@hashhar Could you take a look at this and merge if it looks OK?

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Dec 20, 2022

will we actually have this problem now? This only affects PRs whose base commit is older than when the job was added?

I'm ok with adding this but I think at some point in future this will be redundant?

@MiguelWeezardo
Copy link
Copy Markdown
Member Author

MiguelWeezardo commented Dec 21, 2022

will we actually have this problem now? This only affects PRs whose base commit is older than when the job was added?

I'm ok with adding this but I think at some point in future this will be redundant?

There are still open PRs which are based on older commits. Once these get updated with whatever review comments they had, their authors are in for a surprise when this check fails on them. This will lead to a lot of CI churn, since they will have to discover they need to rebase, and rerun the full CI pipeline after rebase. I'd like to avoid that if possible.

There's also another issue - while working on this I discovered and fixed a bug related to the order in which commits to compile were chosen. My intention was to compile all PR commits except the newest (HEAD), but It turned out that I was compiling every PR commit except the oldest one.

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Dec 22, 2022

Let's create an issue to "re-simplify" this at some point in future.

@hashhar hashhar merged commit 9747b41 into trinodb:master Dec 22, 2022
@github-actions github-actions Bot added this to the 404 milestone Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants