[SPARK-25018] [Infra] Use `Co-authored-by` and `Signed-off-by` git trailer in `merge_spark_pr.py` #21991

dbtsai · 2018-08-03T18:03:46Z

What changes were proposed in this pull request?

In Linux community, Co-authored-by and Signed-off-by git trailer have been used for awhile.

Until recently, Github adopted Co-authored-by to include the work of co-authors in the profile contributions graph and the repository's statistics. It's a convention for recognizing multiple authors, and can encourage people to collaborate in OSS communities.

Git provides a command line tools to read the metadata to know who commits the code to upstream, but it's not as easy as having Signed-off-by as part of the message so developers can find who is the relevant committers who can help with certain part of the codebase easier.

For a single author PR, I purpose to use Authored-by and Signed-off-by, so the message will look like

Authored-by: Author's name <[email protected]>
Signed-off-by: Committer's name <[email protected]>

For a multi-author PR, I purpose to use Lead-authored-by: and Co-authored-by: for the lead author and co-authors. The message will look like

Lead-authored-by: Lead Author's name <[email protected]>
Co-authored-by: CoAuthor's name <[email protected]>
Signed-off-by: Committer's name <[email protected]>

It's also useful to include Reviewed-by: to give credits to the people who participate on the code reviewing. We can add this in the next iteration.

dbtsai · 2018-08-03T21:31:29Z

There are couple other common used git trailers used in linux community, https://git.wiki.kernel.org/index.php/CommitMessageConventions

We may consider to add Reviewed-by: which can be helpful to know who participates the code review.

dbtsai · 2018-08-03T22:23:15Z

dev/merge_spark_pr.py

    # The string "Closes #%s" string is required for GitHub to correctly close the PR
    merge_message_flags += ["-m", "Closes #%s from %s." % (pr_num, pr_repo_desc)]

+    authors = "\n".join(["Co-authored-by: %s" % a for a in distinct_authors])


For single author commit, should we do Authored-by instead of Co-authored-by?

Or we can have the first author as Authored-by like the following.

Authored-by: David <[email protected]> Co-authored-by: Bill <[email protected]>

or leave it as before Author: to be safe?

Do these particular keys have a meaning to github or other tools? if "Author:" is what something is looking for, we'd want to leave it, I presume. But adding more lines for co-authors seems fine. I would have just listed multiple "Author:" lines, but sounds like there's another convention around "Co-authored-by:" that another tool looks for?

Seems that those keys are widely used in Linux community for a long time, and Github adopted Co-authored-by to include the work of cu-authoers in the profile contributions graph and the repository's statistics.

This has to be in the end of the message, and Github will just ignore those keys that are not recognized.

If we want to keep both multiple lines of Author: and Co-authored-by:, the information will be duplicated twice. I would like to purpose to just keep Co-authored-by:. For single developer commit, we can just use Authored-by: for consistency.

SparkQA · 2018-08-03T23:08:34Z

Test build #94161 has finished for PR 21991 at commit 2563311.

This patch fails from timeout after a configured wait of `300m`.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-04T02:40:13Z

Test build #94181 has finished for PR 21991 at commit 6732913.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-08-04T05:46:59Z

+1

felixcheung · 2018-08-04T06:58:13Z

Jenkins, retest this please

felixcheung

SGTM

SparkQA · 2018-08-04T11:24:43Z

Test build #94204 has finished for PR 21991 at commit 6732913.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-06T07:05:02Z

Test build #94265 has finished for PR 21991 at commit fbd8cb4.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-08-06T12:41:43Z

retest this please

HyukjinKwon · 2018-08-06T12:46:17Z

@dbtsai, I sent a PR to your branch dbtsai#3 in case you want to test this and if I understood correctly.

Empty commit to test 'Co-authored-by' and 'Signed-off-by'

dbtsai · 2018-08-06T18:17:25Z

@HyukjinKwon Thanks for pushing a test. I used this script to merge this PR into my own repo. Here is the result. https://github.com/dbtsai/spark/commits/PR_TOOL_MERGE_PR_21991_MASTER As you can see, three authors are properly shown in git status.

I also tested it with single author PR, and here is the result. dbtsai@16dd571

SparkQA · 2018-08-06T18:26:16Z

Test build #94284 has finished for PR 21991 at commit fbd8cb4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-06T21:35:42Z

Test build #94295 has finished for PR 21991 at commit 9016851.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-06T22:13:21Z

Test build #94305 has finished for PR 21991 at commit a69cfb3.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Add manually given primary author at the head of author list

dbtsai · 2018-08-06T22:25:49Z

dev/merge_spark_pr.py

        primary_author = distinct_authors[0]
+    else:
+        # When primary author is specified manually, put it at the head of author list.
+        distinct_authors.insert(0, primary_author)


@viirya good catch. When we put it at the head of the author list, do we need to dedupe it again?

I gave it a thought, but didn't do it. If you think it is necessary, I will push another commit for it.

SparkQA · 2018-08-06T22:31:16Z

Test build #94299 has finished for PR 21991 at commit 4500d60.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-06T23:43:33Z

Test build #94310 has finished for PR 21991 at commit 483d182.

This patch fails from timeout after a configured wait of `300m`.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon

LGTM

HyukjinKwon · 2018-08-07T01:39:00Z

retest this please

dbtsai · 2018-08-07T01:49:43Z

@HyukjinKwon thanks! Is it possible to use this script to merge this PR which has many people involve? A good demonstration of collaboration in the community.

HyukjinKwon · 2018-08-07T02:04:01Z

Yes, I think so. I was about to merge this in that way :-). Seems to me we are good to merge now since the current change is only checked by Python lint which is already passed.

SparkQA · 2018-08-07T02:04:31Z

Test build #94322 has finished for PR 21991 at commit 11887ae.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-08-07T02:05:40Z

Let me leave it to you @dbtsai. I thought you live in a timezone completely different with me.

HyukjinKwon · 2018-08-07T02:29:29Z

Merged to master.

HyukjinKwon · 2018-08-07T02:31:49Z

51bee7a looks good :-)

SparkQA · 2018-08-07T03:26:22Z

Test build #94324 has finished for PR 21991 at commit 272d8fd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-08-07T03:40:24Z

The failed test is FlatMapGroupsWithStateSuite.flatMapGroupsWithState. I saw it fails some times occasionally. I think it should not be related to this change. @HyukjinKwon @dbtsai

SparkQA · 2018-08-07T03:56:46Z

Test build #94323 has finished for PR 21991 at commit b3347f8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-07T05:38:15Z

Test build #94329 has finished for PR 21991 at commit 272d8fd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dbtsai · 2018-08-07T16:42:22Z

Thanks all!

Use co-author-by

2563311

added signed-off-by

6732913

dbtsai changed the title ~~[SPARK-25018] [Infra] Use Co-Authored-By git trailer in merge_spark_pr.py~~ [SPARK-25018] [Infra] Use Co-authored-by and Signed-off-by git trailer in merge_spark_pr.py Aug 3, 2018

dbtsai commented Aug 3, 2018

View reviewed changes

felixcheung approved these changes Aug 4, 2018

View reviewed changes

lead-authored-by

fbd8cb4

srowen approved these changes Aug 6, 2018

View reviewed changes

Empty commit to test 'Co-authored-by' and 'Signed-off-by'

c5cd418

HyukjinKwon mentioned this pull request Aug 6, 2018

Empty commit to test 'Co-authored-by' and 'Signed-off-by' dbtsai/spark#3

Merged

dbtsai added 2 commits August 6, 2018 10:21

Merge pull request #3 from HyukjinKwon/test-script

7dd0173

Empty commit to test 'Co-authored-by' and 'Signed-off-by'

Added new line

4500d60

dbtsai force-pushed the script branch from 9016851 to 4500d60 Compare August 6, 2018 17:47

add thrid author

a69cfb3

fix first author bug

483d182

Add manually given primary author at the head of author list.

cc67c8d

Merge pull request #4 from viirya/script_pr

11887ae

Add manually given primary author at the head of author list

dbtsai commented Aug 6, 2018

View reviewed changes

De-dup primary author from distinct author list.

272d8fd

viirya force-pushed the script branch from b3347f8 to 272d8fd Compare August 6, 2018 23:28

HyukjinKwon approved these changes Aug 7, 2018

View reviewed changes

asfgit closed this in 51bee7a Aug 7, 2018

HyukjinKwon mentioned this pull request Aug 7, 2018

[SPARK-24886][INFRA] Fix the testing script to increase timeout for Jenkins build (from 300m to 340m) #21845

Closed

dbtsai deleted the script branch August 14, 2018 21:03

[SPARK-25018] [Infra] Use Co-authored-by and Signed-off-by git trailer in merge_spark_pr.py #21991

[SPARK-25018] [Infra] Use Co-authored-by and Signed-off-by git trailer in merge_spark_pr.py #21991

Uh oh!

Conversation

dbtsai commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Uh oh!

dbtsai commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbtsai Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen Aug 4, 2018

Choose a reason for hiding this comment

Uh oh!

dbtsai Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 3, 2018

Uh oh!

SparkQA commented Aug 4, 2018

Uh oh!

HyukjinKwon commented Aug 4, 2018

Uh oh!

felixcheung commented Aug 4, 2018

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 4, 2018

Uh oh!

SparkQA commented Aug 6, 2018

Uh oh!

HyukjinKwon commented Aug 6, 2018

Uh oh!

HyukjinKwon commented Aug 6, 2018

Uh oh!

dbtsai commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Aug 6, 2018

Uh oh!

SparkQA commented Aug 6, 2018

Uh oh!

SparkQA commented Aug 6, 2018

Uh oh!

dbtsai Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 6, 2018

Uh oh!

SparkQA commented Aug 6, 2018

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Aug 7, 2018

Uh oh!

dbtsai commented Aug 7, 2018

Uh oh!

HyukjinKwon commented Aug 7, 2018

Uh oh!

SparkQA commented Aug 7, 2018

Uh oh!

HyukjinKwon commented Aug 7, 2018

Uh oh!

HyukjinKwon commented Aug 7, 2018

Uh oh!

HyukjinKwon commented Aug 7, 2018

Uh oh!

[SPARK-25018] [Infra] Use `Co-authored-by` and `Signed-off-by` git trailer in `merge_spark_pr.py` #21991

[SPARK-25018] [Infra] Use `Co-authored-by` and `Signed-off-by` git trailer in `merge_spark_pr.py` #21991

dbtsai commented Aug 3, 2018 •

edited

Loading

dbtsai commented Aug 3, 2018 •

edited

Loading

dbtsai Aug 3, 2018 •

edited

Loading

felixcheung Aug 4, 2018 •

edited

Loading

dbtsai commented Aug 6, 2018 •

edited

Loading