-
Couldn't load subscription status.
- Fork 1.6k
chore: Add script to squash commits in PRs that are ready to merge #5888
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
base: develop
Are you sure you want to change the base?
Conversation
… has been written
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5888 +/- ##
=======================================
Coverage 79.5% 79.5%
=======================================
Files 816 817 +1
Lines 72185 72209 +24
Branches 8297 8275 -22
=======================================
+ Hits 57372 57393 +21
- Misses 14813 14816 +3 🚀 New features to boost your workflow:
|
Co-authored-by: Ed Hennis <[email protected]>
Co-authored-by: Ed Hennis <[email protected]>
|
@ximinez I made a couple of changes to simplify the logic.
|
.github/workflows/on-pr.yml
Outdated
| merge-queue-check: | ||
| if: ${{ contains(github.event.pull_request.labels.*.name, 'MergeQueueCI') }} | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: No-op | ||
| run: true | ||
|
|
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.
I don't think this is necessary. If the flag is set, should-run will be skipped, which causes all the things that depend on it to be skipped, which includes build-test and passed. Skipped counts as a success.
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.
I understood that:
If job 3 depends on job 2, and job 2 depends on job 1, then:
- if job 1 is skipped via if-statement, then jobs 2 and 3 will be skipped, but job 3 is "failingly skipped", i.e. you can't merge if it is required.
- if job 1 runs but job 2 is skipped, then job 3 will be "successfully skipped", because a direct dependency was skipped, allowing the merge if job 3 is required.
But perhaps there's more nuance to it.
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.
I understood that:
If job 3 depends on job 2, and job 2 depends on job 1, then:
* if job 1 is skipped via if-statement, then jobs 2 and 3 will be skipped, but job 3 is "failingly skipped", i.e. you can't merge if it is required. * if job 1 runs but job 2 is skipped, then job 3 will be "successfully skipped", because a direct dependency was skipped, allowing the merge if job 3 is required.But perhaps there's more nuance to it.
I think that all jobs in both scenarios will could as "successfully skipped". That's why we can merge PRs that don't affect any of the files in the changes step.
Required status checks must have a successful, skipped, or neutral status before collaborators can make changes to a protected branch. Required status checks can be checks or commit statuses. For more information, see About status checks.
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.
Ok, I've removed it but wonder if it will then do what I want it to do..
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.
Ok, I've removed it but wonder if it will then do what I want it to do..
Let's try it and see what happens. If it breaks, you can give me a big "I told you so", and resurrect this job.
| gh pr checkout "${pr}" | ||
| git merge ${target} --no-edit | ||
|
|
||
| # TODO: check for conflicts and abort if there are any. |
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.
@Bronek do you have a good command I can use for this? Possibly to combine with the git merge command above that will fail gracefully/detectably if merging cannot be done automatically.
| # the 'SkipRunCI' and 'MergeQueueCI' labels. The former two labels will result | ||
| # in the PR not being allowed to be merged, whereas the latter label will | ||
| # allow it to be merged without running the full CI suite, because the 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.
@ximinez Right now I think the DraftRunCI, SkipRunCI and MergeQueueCI labels will behave identically, with all allowing the PR to either be merged or not to be merged (I'd need to run some tests to confirm which one it is).
I'll have to do some more pondering over how to achieve what is described in the comment.
| if [[ "${COMMIT_TITLE}" =~ ^[^:]+:[^:]+ ]]; then | ||
| if ! [[ "${COMMIT_TITLE}" =~ ^(build|chore|docs|fix|perf|refactor|test):[^:]+ ]]; then |
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.
We're doing more or less the same thing in Clio already.
Could we not reimplement this in a different way in rippled?
https://github.com/XRPLF/clio/blob/develop/.github/workflows/check-pr-title.yml
So, move Clio's implementation in XRPLF/actions, and then reuse it in Clio and rippled
High Level Overview of Change
This PR adds a script to squash all commits in a PR using a provided commit message, and then force-pushes it to the corresponding branch.
To support this new functionality, a
MergeQueueCIand aSkipRunCIlabel are introduced to skip running the CI pipelines which do and do not allow the PR to be merged, respectively. The merge queue pipeline is moved to a separate workflow, as the check to confirm that a PR was squashed with the script made keeping it in the PR workflow complicated.Context of Change
We would like to enable a merge queue in our repository to increase developer velocity. For all intents and purposes, the merge queue will process all PRs that have been added to the queue within a certain time period, and then merge them one by one while ensuring they are brought up to date before doing so. All code is built and tested as usual, and if a failure occurs the affected PR is removed from the queue; this also happens in case of conflicts.
Currently, GitHub does not permit specifying a commit message when using a merge queue, which means that it will provide the default commit message: title = the PR title, description = the list of individual commits followed by a list of authors. We don't like this default message, and would like to adjust it. As it turns out, squashing all commits in a PR, providing the desired commit message, and then force-pushing it to the branch gets us there (except for the list of authors, which are still added; we can live with this).
Although the process to do this is not complicated, there are several steps to be followed. This PR therefore has created a script to make this process as seamless as possible. The script:
The script also works for forks when maintainers have been granted permission to push to them (which is generally needed anyway to allow maintainers to update the branch with the latest changes from the develop branch).
This change skips running the CI pipelines when pushing the updated commit message, because once added to the merge queue the CI pipelines are run again anyway, which must succeed for the PR to be actually merged.
Type of Change
.gitignore, formatting, dropping support for older tooling)