-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-54960][INFRA] Simplify generating contributors of the release process #53728
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
Conversation
JIRA Issue Information=== Improvement SPARK-54960 === This comment was automatically generated by GitHub Actions |
| # The PR number and github username is in the commit message | ||
| # itself and cannot be accessed through any GitHub API | ||
| pr_number = None | ||
| github_username = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably skip the case when github_username is not found. 99% cases github_username should be available but there are other cases like removal of the account, co-authored commits who don't have github accounts, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's skipped, see https://github.com/apache/spark/pull/53728/files#diff-b5c7570e43a02752a0585f7f3de43edfc172d278540864775de7fd41e206139aR167
This function is to list commits, and we should still list all commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that tag or release tag already works like that inside GitHub repository and the rational of this proposal. However, why Apache Software Foundation community enforces that? This sounds a little controversial to me because it's not a perfect solution too.
I think it's more useful to show the github user id for credits, instead of the full name.
If we need this change explicitly, could you send out a simple announcement to dev@spark before merging this, please, @cloud-fan ? To be clear, I would not disagree with this change (if this is exposed once in the ASF mailing list once).
|
@dongjoon-hyun this proposal does not drop full name, it's just to remove human toil to figure out the full name, and read it from the github user profile. Most of the cases we just pick the full name from github user profile, it's very rare that you happen to know this person and can correct his/her full name. |
|
To be clear, I'm not saying about the dropping an information (full name). Instead, it's about enforcing ASF community and contributors to expose additional information. IIUC, there are three fundamental changes in this PR.
Please don't get me wrong. We can do this, but we need to discuss this in the mailing list officially instead of a submarine patch because this PR is also proposed by a company employee whose company seems to have a company policy like the above company-owned ID for some purpose. That's all what I asked here in the previous comment, @cloud-fan . |
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for sharing the proposal to the broader audience, @cloud-fan .
+1 from my side.
|
thanks for the review, merging to master! |
What changes were proposed in this pull request?
One step of the release process is to generate the contributor names and put them in the release notes. The script does a lot of work to find the full name of contributors, but I think it's more useful to show the github user id for credits, instead of the full name.
This PR simplifies this step: we now list contributor names in the form of
github user id (Full Name)or without full name if the github user does not specify full name in the profile.Why are the changes needed?
Simplify release process.
Does this PR introduce any user-facing change?
no
How was this patch tested?
manually, to generate 4.1.0 contributors
Was this patch authored or co-authored using generative AI tooling?
cursor 2.3.29