GH-33005: [Dev] Update the pull request merge script to work with master or main #14533

lafiona · 2022-10-27T19:48:18Z

Overview

The goal of this pull request is to update the GitHub merge script to work with a repository default branch named master or main, as part of the effort to rename the Apache Arrow repository's default branch to main. The parent Jira ticket can be found here.

Implementation

In .github/workflows/dev.yml, get and pass the default branch name to the merge test script.
Added a function named git_default_branch_name to dev/merge_arrow_pr.py.
Used git_default_branch_name to replace hard-coded usages of master in the file.

Testing

Ran the dev/merge_arrow_pr.py in debug mode to confirm that there are no errors when attempting to merge an existing pull request.
Ran test_merge_arrow_pr.py locally to confirm there are no failures.

Future Directions

Updated ARROW-18011 to include updating the hard-coded master value in test_merge_arrow_pr.py.

Notes

Thank you @kevingurney for your help with this pull request!

Closes: [Dev] Update the pull request merge script to work with master or main #33005

assignUser · 2022-12-02T12:23:36Z

@lafiona I see that this has been ready for review for a while. Visibility for the status off PRs is not great at the moment (we constantly >250 open in recent times) so feel free to ping potential reviewers directly (e.g. @raulcd, @jorisvandenbossche, me ...) to move things forward!

assignUser · 2022-12-02T12:24:50Z

(oh and we have recently worked quite a lot on the merge script to enable the use of github issues, so you will need to rebase and fix conflicts :) )

lafiona · 2022-12-06T15:14:14Z

@lafiona I see that this has been ready for review for a while. Visibility for the status off PRs is not great at the moment (we constantly >250 open in recent times) so feel free to ping potential reviewers directly (e.g. @raulcd, @jorisvandenbossche, me ...) to move things forward!

@assignUser thank you for the suggestion to directly ping reviewers! I will keep this in mind.

For this pull request, I'll rebase, fix conflicts, and comment here again. Thank you!

assignUser

Tests look good, I tested it locally in debug mode and on a fork issue: assignUser#6

@lafiona is this done from your end? I would like to push this forward (after another review on such an important part of our infra)

lafiona · 2023-01-07T03:00:23Z

@assignUser Thank you for your quick review! This is all set from my end.

dev/merge_arrow_pr.py

assignUser

👍 Thanks!

raulcd

Thanks for the PR @lafiona . I think the code has some left overs from past workflows and we don't really require the default branch check. I think it's better to simplify it now that we are modifying it.

raulcd · 2023-01-10T08:42:56Z

dev/merge_arrow_pr.py

 def fix_version_from_branch(branch, versions):
    # Note: Assumes this is a sorted (newest->oldest) list of un-released
    # versions
-    if branch == "master":
+    if branch == git_default_branch_name():
        return versions[-1]
    else:


I think this else is never executed because we never call get_candidate_fix_version with merge_branches. We always use the default ('master',). We could simplify the script to just return versions[-1] on this function and remove all the logic based on the branch.
The merge script uses the GitHub API and that will already point the PR to the correct default branch.
We don't use branches prefixed branch-. This code is 7 years old so it is probably a left over from an old workflow.

@raulcd , thanks for this suggestion and confirmation that it's leftover code. I'll remove the conditional here as you suggested.

assignUser · 2023-01-10T22:43:43Z

dev/merge_arrow_pr.py

+
+    if default_branch_name is None:
+        try:
+            default_reference = run_cmd(


lint:
/arrow/dev/merge_arrow_pr.py:119:13: F841 local variable 'default_reference' is assigned to but never used

Thank you for pointing this out! I have pushed the fix for this issue, however, I am now seeing an error that I did not see before, when running the script in debug mode:

`Traceback` (most recent call last): File "/Users/fionala/r2023b/arrow/dev/merge_arrow_pr.py", line 747, in <module> cli() File "/Users/fionala/r2023b/arrow/dev/merge_arrow_pr.py", line 742, in cli pr.issue.resolve(fix_version, issue_comment, pr.body) File "/Users/fionala/r2023b/arrow/dev/merge_arrow_pr.py", line 187, in resolve resolve = [x for x in self.jira_con.transitions(self.jira_id) IndexError: list index out of range

I'm working on debugging this to determine if it is related to the most recent changes I made.

I think that it's not related.
I think that it's related to Jira -> GitHub migration.
@rok What do you think about this?

Huh, that might be because Jira was locked down today - no changes to issues are possible going forward. But @raulcd still merged #33611 so I'm not sure. Could you try again @lafiona ?

a rebase to integrate the changes made to the script today might also be good

Thank you for the insight about Jira being locked down, I was able to run the test without the error, just now. And I have rebased to integrate the most recent changes.

This change is good to go on my end now. Thank you everyone for all your help!

github-actions · 2023-01-11T21:24:17Z

⚠️ GitHub issue #33005 has been automatically assigned in GitHub to PR creator.

github-actions · 2023-01-11T21:24:54Z

⚠️ GitHub issue #33005 has been automatically assigned in GitHub to PR creator.

…ue to replace hard coded 'master' values. Co-authored-by: Kevin Gurney <kgurney@mathworks.com>

Co-authored-by: Kevin Gurney <kgurney@mathworks.com>

…e, pass this value to the merge script.

…ontaining a head reference to get the default branch

…ow_pr.py

Co-authored-by: Kevin Gurney <kgurney@mathworks.com> Co-authored-by: Sreehari Hegden <sreehari.hegden@gmail.com>

…ault branch name via git rev-parse.

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

…and unused input argument in fix_version_from_branch()

raulcd · 2023-01-12T18:41:46Z

dev/merge_arrow_pr.py

+    if merge_branches is None:
+        merge_branches = (git_default_branch_name(),)


Thanks @lafiona for the changes! Only one more thing, now merge_branches is never used, we can get rid of it and we can also get rid of the new function created to retrieve the git_default_branch_name because the script is never calling that function once we remove merge_branches. That makes sense as we use the GitHub API to merge the PR and we use the base branch defined on the PR, not the one on this script.

Hi @raulcd, I apologize that I didn't see this earlier, it's tied to the feedback that you already left yesterday! And I've removed the input argument as well as references to merge_branches in the function body. Thanks for pointing it out!

Thank you very much! I'll wait for the linter to pass and I'll merge after successful build

…version().

raulcd

Thank you very much for the changes I think they have been great! One step closer to migrate the default branch! I'll merge this with this two suggestions. I'll use this branch merge script to test it.

dev/merge_arrow_pr.py

.github/workflows/dev.yml

dev/merge_arrow_pr.py

Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>

dev/merge_arrow_pr.py

Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>

ursabot · 2023-01-14T05:36:32Z

Benchmark runs are scheduled for baseline = 2e3683b and contender = 3d26a43. 3d26a43 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.39% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 3d26a433 ec2-t3-xlarge-us-east-2
[Finished] 3d26a433 test-mac-arm
[Finished] 3d26a433 ursa-i9-9960x
[Finished] 3d26a433 ursa-thinkcentre-m75q
[Finished] 2e3683b7 ec2-t3-xlarge-us-east-2
[Failed] 2e3683b7 test-mac-arm
[Finished] 2e3683b7 ursa-i9-9960x
[Finished] 2e3683b7 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

This comment was marked as outdated.

Sign in to view

lafiona force-pushed the ARROW-17777 branch from b91d37a to 5f98f88 Compare November 16, 2022 16:24

lafiona marked this pull request as ready for review November 17, 2022 22:20

jorisvandenbossche added the Component: Developer Tools label Dec 2, 2022

lafiona force-pushed the ARROW-17777 branch from 6d3c5e2 to 00ac13f Compare January 6, 2023 21:46

assignUser reviewed Jan 7, 2023

View reviewed changes

assignUser requested review from kou, kszucs, pitrou and raulcd January 7, 2023 02:03

kou reviewed Jan 7, 2023

View reviewed changes

dev/merge_arrow_pr.py Outdated Show resolved Hide resolved

lafiona force-pushed the ARROW-17777 branch from 06f5bbf to 84b7fac Compare January 9, 2023 22:15

assignUser requested a review from kou January 9, 2023 23:22

assignUser approved these changes Jan 9, 2023

View reviewed changes

raulcd reviewed Jan 10, 2023

View reviewed changes

assignUser requested changes Jan 10, 2023

View reviewed changes

lafiona force-pushed the ARROW-17777 branch from 7d85dae to 29bb5bf Compare January 11, 2023 02:57

lafiona requested review from assignUser, kou and raulcd and removed request for kou, kszucs, pitrou and assignUser January 11, 2023 03:07

asfimport mentioned this pull request Jan 10, 2023

[Dev] Update the pull request merge script to work with master or main #33005

Closed

apache deleted a comment from github-actions bot Jan 11, 2023

lafiona and others added 11 commits January 12, 2023 13:33

Add function to compute the default branch name from git, use the val…

e23d239

…ue to replace hard coded 'master' values. Co-authored-by: Kevin Gurney <kgurney@mathworks.com>

Use python for processing the default branch reference from git.

69f64c7

Co-authored-by: Kevin Gurney <kgurney@mathworks.com>

Use the git environment in the yml file to get the default branch nam…

70294b3

…e, pass this value to the merge script.

Enable users who are calling the merge script from a git repository c…

a1de551

…ontaining a head reference to get the default branch

Run linting

af9a04a

Apply branch renaming changes to refactoring changes in dev/merge_arr…

292694e

…ow_pr.py

Add default value for merge_branches in get_candidate_fix_version

c91010d

Co-authored-by: Kevin Gurney <kgurney@mathworks.com> Co-authored-by: Sreehari Hegden <sreehari.hegden@gmail.com>

Added rstrip command to remove newline character after retrieving def…

51aae04

…ault branch name via git rev-parse.

Update dev/merge_arrow_pr.py to not use lstrip()

e3582b7

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Fix startswith call to use the correct string and remove unused code …

702f8e4

…and unused input argument in fix_version_from_branch()

Modify call site of fix_version_from_branch to return a scalar value.

f264faa

lafiona force-pushed the ARROW-17777 branch from 7544ea0 to f264faa Compare January 12, 2023 18:34

raulcd reviewed Jan 12, 2023

View reviewed changes

Remove merge_branches in argument list and body of get_candidate_fix_…

db2beaf

…version().

raulcd approved these changes Jan 12, 2023

View reviewed changes

dev/merge_arrow_pr.py Outdated Show resolved Hide resolved

.github/workflows/dev.yml Outdated Show resolved Hide resolved

raulcd reviewed Jan 12, 2023

View reviewed changes

dev/merge_arrow_pr.py Show resolved Hide resolved

lafiona and others added 4 commits January 12, 2023 15:02

Remove extra newline.

6eb674c

Update dev/merge_arrow_pr.py to avoid importing "warnings" module.

677ad20

Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>

Update dev/merge_arrow_pr.py

d829c55

Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>

Update .github/workflows/dev.yml

992eafe

Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>

raulcd reviewed Jan 12, 2023

View reviewed changes

dev/merge_arrow_pr.py Show resolved Hide resolved

Update dev/merge_arrow_pr.py

f59a539

Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>

raulcd merged commit 3d26a43 into apache:master Jan 12, 2023

kevingurney deleted the ARROW-17777 branch August 21, 2023 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-33005: [Dev] Update the pull request merge script to work with master or main #14533

GH-33005: [Dev] Update the pull request merge script to work with master or main #14533

lafiona commented Oct 27, 2022 •

edited by assignUser

This comment was marked as outdated.

This comment was marked as outdated.

assignUser commented Dec 2, 2022

assignUser commented Dec 2, 2022

lafiona commented Dec 6, 2022

assignUser left a comment

lafiona commented Jan 7, 2023

assignUser left a comment

raulcd left a comment

raulcd Jan 10, 2023

lafiona Jan 10, 2023

assignUser Jan 10, 2023

lafiona Jan 11, 2023

kou Jan 11, 2023

rok Jan 11, 2023 •

edited

assignUser Jan 11, 2023

lafiona Jan 12, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

raulcd Jan 12, 2023

lafiona Jan 12, 2023

raulcd Jan 12, 2023

raulcd left a comment

ursabot commented Jan 14, 2023

		if merge_branches is None:
		merge_branches = (git_default_branch_name(),)

GH-33005: [Dev] Update the pull request merge script to work with master or main #14533

GH-33005: [Dev] Update the pull request merge script to work with master or main #14533

Conversation

lafiona commented Oct 27, 2022 • edited by assignUser

Overview

Implementation

Testing

Future Directions

Notes

This comment was marked as outdated.

This comment was marked as outdated.

assignUser commented Dec 2, 2022

assignUser commented Dec 2, 2022

lafiona commented Dec 6, 2022

assignUser left a comment

Choose a reason for hiding this comment

lafiona commented Jan 7, 2023

assignUser left a comment

Choose a reason for hiding this comment

raulcd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rok Jan 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulcd left a comment

Choose a reason for hiding this comment

ursabot commented Jan 14, 2023

lafiona commented Oct 27, 2022 •

edited by assignUser

rok Jan 11, 2023 •

edited