Allow auto merge binding PRs #988

qinsoon · 2023-10-18T03:38:03Z

This PR allows auto merging related binding PRs once the mmtk-core PR is merged. It also adds some docs for MMTk team about merging process. The PR closes #205.

qinsoon · 2023-10-18T04:06:51Z

binding-refs
OPENJDK_BINDING_REPO=qinsoon/mmtk-openjdk
OPENJDK_BINDING_REF=update-mmtk-core-pr-988

qinsoon · 2023-10-18T06:25:18Z

Most of the workflow is tested, and works fine. See https://github.com/mmtk/mmtk-core/actions/runs/6556991187. The workflows can update the mmtk dependency and turn on auto merging in the testing PR in OpenJDK (mmtk/mmtk-openjdk#254).

One thing that is not tested yet is the merge commit hash. It is acquired with github.event.pull_request.merge_commit_sha. According to the documents (https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request), the merge_commit_sha should be "the SHA of the squashed commit on the base branch". We will only know if it works fine after we actually merge a pull request.

qinsoon · 2023-10-19T04:41:28Z

I think the current approach may not work if a mmtk-core PR is opened from a fork. The workflow needs to access secrets.CI_ACCESS_TOKEN which is probably not available if the PR is opened from a fork. I can try change the trigger from "closed pull_request" to "push to master". In that case, the workflow is always triggered from the upstream repo.

wks

There are some problems in the Python script replace-mmtk-dep.py that should be changed.

.github/scripts/replace-mmtk-dep.py

This reverts commit db2e4b4.

qinsoon · 2023-10-20T00:44:51Z

I think the current approach may not work if a mmtk-core PR is opened from a fork. The workflow needs to access secrets.CI_ACCESS_TOKEN which is probably not available if the PR is opened from a fork. I can try change the trigger from "closed pull_request" to "push to master". In that case, the workflow is always triggered from the upstream repo.

I changed the workflow trigger. It should be fine now.

wks

LGTM

This reverts commit b5af19f.

qinsoon · 2023-10-20T02:00:25Z

I changed the way how to update the lock file. It now uses cargo build instead of cargo generate-lockfile, as genearte-lockfile may update the version of dependencies. If a dependency does not comply with semantic versioning, we may end up with a broken build. Use cargo build won't have such an issue.

I don't plan to make any further changes to the PR unless requested.

This PR updates mmtk-core to mmtk/mmtk-core#988. Once mmtk/mmtk-core#988 is merged, this PR should be updated to the new mmtk-core commit, and will be auto merged (if all the requirements for auto merging is met). --------- Co-authored-by: mmtkgc-bot <[email protected]>

We saw the auto merging workflow (added in #988) failed in https://github.com/mmtk/mmtk-core/actions/runs/6587030083/job/17896533403. The error message was `GraphQL: Head branch is out of date. Review and try the merge again. (mergePullRequest)`. It is likely caused by the issue https://github.com/orgs/community/discussions/24462: When a PR gets updated, Github has a process to determine if the PR is mergable. If we attempt to enable auto merge before the process is done, the merge will fail with the error message above. This PR adds a delay and retry for enabling auto merging.

Allow auto merge binding PRs

2e15fe3

qinsoon mentioned this pull request Oct 18, 2023

DONT MERGE: Testing auto merging binding PRs mmtk/mmtk-openjdk#254

Closed

qinsoon added 2 commits October 18, 2023 04:11

Update docs

c250e73

Make the workflow run for an open PR for testing

a70d0ee

qinsoon force-pushed the auto-merge-binding-prs branch from 99e68cd to a70d0ee Compare October 18, 2023 04:15

qinsoon added 8 commits October 18, 2023 04:19

Skip installing github cli

c50b40e

Inherit secrets and supply token for gh cli

7a67fde

Fix usage of replace-mmtk-dep

2f14a97

Fix manifest path

15aa0f7

Use CI_ACCESS_TOKEN for git push

75b122a

Checkout binding repo with CI_ACCESS_TOKEN

ba557b4

Add GITHUB_TOKEN for auto merging

dd30c90

Extract PR number correctly

d098fb9

qinsoon force-pushed the auto-merge-binding-prs branch from 489cdf1 to d098fb9 Compare October 18, 2023 05:48

qinsoon added 2 commits October 18, 2023 05:50

Auto merge with squash

79bf432

Fix commit user email

f73342e

qinsoon force-pushed the auto-merge-binding-prs branch from b5a7ec6 to f73342e Compare October 18, 2023 06:02

Add jobs for each binding

0fe3355

Revert test trigger. Create a new toml node for mmtk dependency.

f139324

qinsoon mentioned this pull request Oct 18, 2023

Update to mmtk-core PR #988 mmtk/mmtk-openjdk#255

Merged

qinsoon marked this pull request as ready for review October 18, 2023 06:58

qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Oct 18, 2023

qinsoon added 2 commits October 18, 2023 23:58

Fix the change in replace-mmtk-dep.py that breaks Ruby tests

cad1825

Fix an issue in merge-check.yml. Ruby tests are not ignored properly

12c8221

qinsoon requested review from wks and caizixian October 19, 2023 04:23

wks requested changes Oct 19, 2023

View reviewed changes

.github/scripts/replace-mmtk-dep.py Outdated Show resolved Hide resolved

.github/scripts/replace-mmtk-dep.py Outdated Show resolved Hide resolved

.github/scripts/replace-mmtk-dep.py Outdated Show resolved Hide resolved

qinsoon added 5 commits October 19, 2023 23:58

Use dash instead of underscore in optional args

a0d8483

Preserve formatting when we update git/rev. Remove unnecessary store.

f12501f

Change the trigger from "closed pull request" to "push to master"

2385863

Trigger the workflow on this push

db2e4b4

Revert "Trigger the workflow on this push"

e90709f

This reverts commit db2e4b4.

wks approved these changes Oct 20, 2023

View reviewed changes

qinsoon added 4 commits October 20, 2023 01:43

Add auto/manual merging process in the doc

c3f21cc

Use cargo build instead of cargo generate-lockfile to update lock file.

26e37e7

Change the trigger to test again

b5af19f

Revert "Change the trigger to test again"

aeb54a8

This reverts commit b5af19f.

qinsoon added this pull request to the merge queue Oct 20, 2023

Merged via the queue into master with commit 06a2e5d Oct 20, 2023
20 checks passed

qinsoon deleted the auto-merge-binding-prs branch October 20, 2023 11:20

qinsoon mentioned this pull request Oct 22, 2023

Apply a delay before we enable auto merge on a PR #992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow auto merge binding PRs #988

Allow auto merge binding PRs #988

qinsoon commented Oct 18, 2023 •

edited

Loading

qinsoon commented Oct 18, 2023 •

edited

Loading

qinsoon commented Oct 18, 2023

qinsoon commented Oct 19, 2023

wks left a comment

qinsoon commented Oct 20, 2023

wks left a comment

qinsoon commented Oct 20, 2023

Allow auto merge binding PRs #988

Allow auto merge binding PRs #988

Conversation

qinsoon commented Oct 18, 2023 • edited Loading

qinsoon commented Oct 18, 2023 • edited Loading

qinsoon commented Oct 18, 2023

qinsoon commented Oct 19, 2023

wks left a comment

Choose a reason for hiding this comment

qinsoon commented Oct 20, 2023

wks left a comment

Choose a reason for hiding this comment

qinsoon commented Oct 20, 2023

qinsoon commented Oct 18, 2023 •

edited

Loading

qinsoon commented Oct 18, 2023 •

edited

Loading