Skip to content
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

ARROW-13564: [Dev] Check individual commit messages for "Co-authored-by:" tags when integrating a pull request #12693

Closed
wants to merge 5 commits into from

Conversation

lafiona
Copy link
Contributor

@lafiona lafiona commented Mar 22, 2022

Overview

The goal of this pull request is to add functionality to dev/merge_arrow_pr.py, that checks individual commit messages for Co-authored-by: tags, when creating the squashed commit message when merging a pull request.

This will ensure that all authors are included in the final commit message, to make sure they are acknowledged for their contributions.

Implementation

  1. Use the trailers options of git log to check for the Co-authored-by trailer. The equivalent git command is:
    git log HEAD..<pull-request-branch> --pretty="%(trailers:key=Co-authored-by,valueonly)"
  2. For each commit in <pull-request-branch>, this command prints the value of any Co-authored-by trailers, which should be in the form of <name> <<email-address>>.
  3. The values returned by this command are appended to the list of all commit authors.
  4. Added a comment to set PR_REMOTE_NAME if the user has not set a remote named apache to git@github.com:apache/arrow.git.

Testing

I qualified the changes to merge_arrow_pr.py by taking the the following steps:

  1. Created a branch for this change, ARROW-13564 and corresponding pull request.
  2. Add a commit that has multiple Co-authors.
  3. Set the following environment variables:
    • DEBUG 1
    • PR_REMOTE_NAME upstream (my remote to git@github.com:apache/arrow.git is named upstream)
  4. Run the merge script, merge_arrow_pr.py, following the usage text.
  5. The output from the merge script:
-m
Closes #12693 from lafiona/ARROW-13564
-m
Lead-authored-by: Fiona La <fionala@mathworks.com>
Co-authored-by: Fiona La <fionala7@gmail.com>
Co-authored-by: Kevin Gurney <kgurney@mathworks.com>
Signed-off-by: Fiona La <fionala@mathworks.com>

Merge complete (local ref PR_TOOL_MERGE_PR_12693_MASTER). Push to upstream? (y/n): n

Notes

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

@github-actions
Copy link

@lidavidm
Copy link
Member

It seems you just need to git remote add apache git@github.com:apache/arrow.git (the merge script assumes a remote named apache)

Alternatively, set PR_REMOTE_NAME, e.g. env PR_REMOTE_NAME=origin ./dev/merge_arrow_pr.py 12682

@lafiona lafiona force-pushed the ARROW-13564 branch 2 times, most recently from 3dc39a0 to f7c164d Compare March 23, 2022 14:18
@lafiona
Copy link
Contributor Author

lafiona commented Mar 23, 2022

Thank you for your help with this, @lidavidm ! I was able to verify that the change adds Co-authors to the squashed commit that will be used for the pull request. I've updated the usage text to include the two environment variables that were helpful for my workflow. And I've also updated the description for this pull request.

@lidavidm
Copy link
Member

Thanks!

It looks the Github incident today means CI didn't fully trigger on this PR - do you mind rebasing to make sure it all runs?

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Do you see a good way to add a test for this in https://github.com/apache/arrow/blob/master/dev/test_merge_arrow_pr.py ? Though it seems we don't currently test merge

@lidavidm
Copy link
Member

@lafiona did you mean to push the matlab commit to this PR?

@lafiona
Copy link
Contributor Author

lafiona commented Mar 28, 2022

@lidavidm , I apologize for the MATLAB commit! I did not mean to push it to this branch.

I've rebased the correct commits onto apache/arrow master to trigger the CI. I am still investigating adding tests to test_merge_arrow_pr.py and will update here when I have a solution.

@lidavidm
Copy link
Member

To be fair there might not be too good a way to test it, at least without mocking, or setting up a fake git repo to manipulate (and it certainly doesn't seem to test it right now)

@lafiona
Copy link
Contributor Author

lafiona commented Mar 28, 2022

I think you're right that merge cannot be tested unless there is a git repo, that the test script can create commits and pull requests on, or git mocking infrastructure. I can create a separate Jira ticket for capturing this work.

@lidavidm
Copy link
Member

Thanks. (Sorry, didn't mean to send you down a wild rabbit chase.)

GitHub Actions seem a little backed up today, but LGTM.

@lidavidm lidavidm closed this in 919d113 Mar 28, 2022
@ursabot
Copy link

ursabot commented Mar 28, 2022

Benchmark runs are scheduled for baseline = d327f69 and contender = 919d113. 919d113 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
[Finished ⬇️0.29% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.34% ⬆️1.15%] 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants