-
Notifications
You must be signed in to change notification settings - Fork 899
fix: check if organization and requires sign off commits #2896
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
base: main
Are you sure you want to change the base?
fix: check if organization and requires sign off commits #2896
Conversation
|
Hi @steveteuber, I was reading into this issue and was wondering the following: |
|
@ArlonAntonius Yes, correct. After #2763 was merged, this issue now occurs only when updating a repository. |
nickfloyd
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.
Let me know what you think about my comment and thank you for this change ❤️
| // When the organization has "Require contributors to sign off on web-based commits" enabled, | ||
| // the API doesn't allow you to send `web_commit_signoff_required` or it returns a 422 error. | ||
| // As a workaround, check if the organization requires it, and if so, remove it from the request. | ||
| if meta.(*Owner).IsOrganization && meta.(*Owner).IsWebCommitSignoffRequired { |
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 removing the d.HasChange("web_commit_signoff_required") conditional might be problematic. This change might cause issues when archiving repositories in organizations that don't have signoff required but the repository itself has it set, potentially causing a 422. Check me on that though.
| repoReq := resourceGithubRepositoryObject(d) | ||
| // Always remove `web_commit_signoff_required` when archiving, to avoid 422 error | ||
| repoReq.WebCommitSignoffRequired = nil | ||
| if meta.(*Owner).IsOrganization && meta.(*Owner).IsWebCommitSignoffRequired { |
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.
Same as above.
This is a follow-up pull request to fix the issue described in #2077.
Before the change?
There is a bug in the GitHub API 2022-11-28 version that causes a 422 error when updating a repository within an organization that has "Require sign off on web-based commits" enabled.
After the change?
On a repository update, we check if the organization has "Require sign off on web-based commits" enabled.
If it's enabled, we do not send the web_commit_signoff_required field in the update request to avoid the 422 error.
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!