You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/27 19:48:20 UTC
[GitHub] [arrow] lafiona opened a new pull request, #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
lafiona opened a new pull request, #14533:
URL: https://github.com/apache/arrow/pull/14533
# Overview
The goal of this pull request is to 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](https://issues.apache.org/jira/browse/ARROW-15689).
# Implementation
- Added a function named `git_default_branch_name` to `dev/merge_arrow_pr.py`.
- Used the function to replace hard-coded usages of `master` in the file.
# Testing
- Ran the merge script in debug mode
# Future Directions
# Notes
Thank you @kevingurney for your help with this pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lafiona commented on a diff in pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
lafiona commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1066533847
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,34 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lafiona commented on pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
lafiona commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1374360160
@assignUser Thank you for your quick review! This is all set from my end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1067531825
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,34 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
Review Comment:
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1379502002
* Closes: #33005
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on a diff in pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
assignUser commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1066417521
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,34 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
Review Comment:
lint:
`/arrow/dev/merge_arrow_pr.py:119:13: F841 local variable 'default_reference' is assigned to but never used`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lafiona commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
lafiona commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1068595563
##########
dev/merge_arrow_pr.py:
##########
@@ -278,8 +299,11 @@ def show(self):
def get_candidate_fix_version(mainline_versions,
- merge_branches=('master',),
+ merge_branches=None,
maintenance_branches=()):
+ if merge_branches is None:
+ merge_branches = (git_default_branch_name(),)
Review Comment:
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1382667163
Benchmark runs are scheduled for baseline = 2e3683b78e896cb7f9524a8c55fdfb2026b6f192 and contender = 3d26a4330594b6d6ce21a7dc0cb5a77367b98cfd. 3d26a4330594b6d6ce21a7dc0cb5a77367b98cfd is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/d0fb8b378dda4d9cbed92a3a97493eb5...1469ba056ce34f7786b276a0a867927a/)
[Failed :arrow_down:0.39% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/e4d522eaf75a41f8a33ea8096450b677...b5d4d4b71d4045a99bff0850a963a468/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/01f807f3841846d4938853005e8fa487...b69dd67d2de44ae1a7c6ed3dd2188fae/)
[Finished :arrow_down:0.31% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/57193771943948819340464d5343bbc9...e1e0c9e1754c4424955c2fb241774c08/)
Buildkite builds:
[Finished] [`3d26a433` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2170)
[Finished] [`3d26a433` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2193)
[Finished] [`3d26a433` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2166)
[Finished] [`3d26a433` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2187)
[Finished] [`2e3683b7` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2169)
[Failed] [`2e3683b7` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2192)
[Finished] [`2e3683b7` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2165)
[Finished] [`2e3683b7` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2186)
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14533: GH-31142: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1379501053
* Closes: #31142
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
raulcd commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1066837521
##########
dev/merge_arrow_pr.py:
##########
@@ -287,7 +311,7 @@ def version_tuple(x):
mainline_versions = [v for v in mainline_versions
if f"maint-{v}" not in maintenance_branches]
default_fix_versions = [
- fix_version_from_branch(x, mainline_versions)
+ fix_version_from_branch(mainline_versions)
for x in merge_branches]
return default_fix_versions[0]
Review Comment:
As we are not using `x` the list comprehension is not needed anymore. We can update this to be:
```
return fix_version_from_branch(mainline_versions)
```
We can also remove the `merge_branches` variable definition as is not used anymore and remove it from the signature of the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1379502042
:warning: GitHub issue #33005 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
raulcd commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1068614500
##########
dev/merge_arrow_pr.py:
##########
@@ -302,12 +298,9 @@ def version_tuple(x):
mainline_versions = [v for v in mainline_versions
if f"maint-{v}" not in maintenance_branches]
- default_fix_versions = [
- fix_version_from_branch(x, mainline_versions)
- for x in merge_branches]
-
- return default_fix_versions[0]
+ default_fix_versions = fix_version_from_branch(mainline_versions)
+ return default_fix_versions
Review Comment:
The removed empty line is missing. Flake8 expects 2 empty lines between function definitions
```suggestion
return default_fix_versions
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1064051564
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,32 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
+ "git rev-parse --abbrev-ref origin/HEAD")
+ default_branch_name = default_reference.lstrip("origin/")
Review Comment:
Do we need `rstrip()` here to remove new line?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
assignUser commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1335160534
(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 :) )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1294023806
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
raulcd commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1065482420
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,33 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
+ "git rev-parse --abbrev-ref origin/HEAD")
+ default_branch_name = default_reference.lstrip("origin/")
+ default_branch_name = default_branch_name.rstrip()
+ except subprocess.CalledProcessError:
+ # TODO: ARROW-18011 to track changing the hard coded default
+ # value from "master" to "main".
+ default_branch_name = "master"
+ warnings.warn('Unable to determine default branch name: '
+ 'MERGE_SCRIPT_DEFAULT_BRANCH_NAME environment '
+ 'variable is not set. Git repository does not '
+ 'contain a \'refs/remotes/origin/HEAD\'reference. '
+ ' Setting the default branch name to ' +
+ default_branch_name, RuntimeWarning)
+
+ return default_branch_name
+
+
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:
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
assignUser commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1335159374
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
raulcd commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1068591997
##########
dev/merge_arrow_pr.py:
##########
@@ -109,15 +110,10 @@ def strip_ci_directives(commit_message):
# commit message
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
-
-def fix_version_from_branch(branch, versions):
+def fix_version_from_branch(versions):
Review Comment:
There's a minor linting issue missing an extra empty line here:
```suggestion
def fix_version_from_branch(versions):
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lafiona commented on pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
lafiona commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1378201764
Hi all, my apologies for the notifications: as I was trying to resolve the change requested by @assignUser, I accidentally unrequested review from all other previously reviewed reviewers. As I am trying to add each reviewer back, I can only add one at a time.
What is the recommended way to revert the review request to the previous state?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1066634188
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,34 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
Review Comment:
I think that it's not related.
I think that it's related to Jira -> GitHub migration.
@rok What do you think about this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14533: GH-31142: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1379501094
:warning: GitHub issue #31142 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1379502711
:warning: GitHub issue #33005 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd merged pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
raulcd merged PR #14533:
URL: https://github.com/apache/arrow/pull/14533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lafiona commented on a diff in pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
lafiona commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1065152006
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,32 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
+ "git rev-parse --abbrev-ref origin/HEAD")
+ default_branch_name = default_reference.lstrip("origin/")
Review Comment:
Thank you for your suggestion @kou. There is a newline character that I've added an `rstrip()` call to remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
assignUser commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1067533301
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,34 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
Review Comment:
a rebase to integrate the changes made to the script today might also be good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1294023778
https://issues.apache.org/jira/browse/ARROW-17777
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1067531825
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,34 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
Review Comment:
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..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lafiona commented on a diff in pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
lafiona commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1066400790
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,33 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
+ "git rev-parse --abbrev-ref origin/HEAD")
+ default_branch_name = default_reference.lstrip("origin/")
+ default_branch_name = default_branch_name.rstrip()
+ except subprocess.CalledProcessError:
+ # TODO: ARROW-18011 to track changing the hard coded default
+ # value from "master" to "main".
+ default_branch_name = "master"
+ warnings.warn('Unable to determine default branch name: '
+ 'MERGE_SCRIPT_DEFAULT_BRANCH_NAME environment '
+ 'variable is not set. Git repository does not '
+ 'contain a \'refs/remotes/origin/HEAD\'reference. '
+ ' Setting the default branch name to ' +
+ default_branch_name, RuntimeWarning)
+
+ return default_branch_name
+
+
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:
Review Comment:
@raulcd , thanks for this suggestion and confirmation that it's leftover code. I'll remove the conditional here as you suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
raulcd commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1068489797
##########
dev/merge_arrow_pr.py:
##########
@@ -278,8 +299,11 @@ def show(self):
def get_candidate_fix_version(mainline_versions,
- merge_branches=('master',),
+ merge_branches=None,
maintenance_branches=()):
+ if merge_branches is None:
+ merge_branches = (git_default_branch_name(),)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lafiona commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
lafiona commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1068486909
##########
dev/merge_arrow_pr.py:
##########
@@ -110,10 +111,34 @@ def strip_ci_directives(commit_message):
return _REGEX_CI_DIRECTIVE.sub('', commit_message)
+def git_default_branch_name():
+ default_branch_name = os.getenv("MERGE_SCRIPT_DEFAULT_BRANCH_NAME")
+
+ if default_branch_name is None:
+ try:
+ default_reference = run_cmd(
Review Comment:
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lafiona commented on pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
lafiona commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1339534400
> @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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on pull request #14533: ARROW-17777: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
assignUser commented on PR #14533:
URL: https://github.com/apache/arrow/pull/14533#issuecomment-1378202766
This is a failing of th GitHub ui with the small arrow-circle you can only re-request one revised and it will automatically un-request the other ones. Multi request works via the drop-down (which you might not have access to?) I will request the reviews :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
raulcd commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1068601381
##########
dev/merge_arrow_pr.py:
##########
@@ -278,8 +299,11 @@ def show(self):
def get_candidate_fix_version(mainline_versions,
- merge_branches=('master',),
+ merge_branches=None,
maintenance_branches=()):
+ if merge_branches is None:
+ merge_branches = (git_default_branch_name(),)
Review Comment:
Thank you very much! I'll wait for the linter to pass and I'll merge after successful build
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] raulcd commented on a diff in pull request #14533: GH-33005: [Dev] Update the pull request merge script to work with master or main
Posted by GitBox <gi...@apache.org>.
raulcd commented on code in PR #14533:
URL: https://github.com/apache/arrow/pull/14533#discussion_r1068584129
##########
dev/merge_arrow_pr.py:
##########
@@ -43,6 +43,7 @@
import sys
import requests
import getpass
+import warnings
Review Comment:
```suggestion
```
##########
.github/workflows/dev.yml:
##########
@@ -108,4 +108,6 @@ jobs:
ci/scripts/release_test.sh $(pwd)
- name: Run Merge Script Test
shell: bash
+ env:
+ MERGE_SCRIPT_DEFAULT_BRANCH_NAME: ${{ github.event.repository.default_branch }}
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org