fix: Ignore unset AllowedMergeMethods field
#3905
Conversation
Signed-off-by: Steve Hipwell <[email protected]>
gmlewis
changed the title
AllowedMergeMethods field
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3905 +/- ##
=======================================
Coverage 92.44% 92.44%
=======================================
Files 203 203
Lines 14927 14927
=======================================
Hits 13799 13799
Misses 926 926
Partials 202 202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @stevehipwell!
LGTM.
Merging.
| // PullRequestRuleParameters represents the pull_request rule parameters. | ||
| type PullRequestRuleParameters struct { | ||
| AllowedMergeMethods []PullRequestMergeMethod `json:"allowed_merge_methods"` | ||
| AllowedMergeMethods []PullRequestMergeMethod `json:"allowed_merge_methods,omitempty"` |
There was a problem hiding this comment.
I'm curious, can we use omitzero here?
There was a problem hiding this comment.
I'm not sure why we'd want to? This field can either be missing or it needs to have at least 1 value.
There was a problem hiding this comment.
The unit test does show that this field is being properly omitted when empty. In hindsight, adding a second unit test demonstrating BOTH behaviors would be beneficial.
There was a problem hiding this comment.
Oh, never mind. It does indeed look like the table-driven tests also include that behavior, so I think we are all good here.
3 participants
This PR fixes a bug where the merge methods for a pull request ruleset rule always need to be defined instead of being able to be unset.