-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Issue with Branch Protection fields #2976
Comments
Have a look at #2467 (comment) I would have to check the code and the api again as I haven't used this in a long time but I think the omitempty should not be there and instead the client needs to set null explicitly or maybe the field needs to be a pointer to a list instead of a list. I also remember that one of these use cases was deprecated by GitHub but I might be mixing things here. @viqueen are you using the |
I think, if the Here's my vote: remove the &github.RequiredStatusChecks{
Checks: []*github.RequiredStatusCheck{},
Contexts: []string{"hello-world"},
Strict: true,
}, Basically, If people agree with this consensus, I'd be fine making a PR. |
Why is the |
This is what's written for update branch protection. Oh Github's docs are misleading and confusing. I feel like they should've made checks required and not contexts. Anyway, i guess to match what their API is doing, we should remove |
I saw that originally too. The docs for |
OK, so I'm leaning toward the minimally-invasive solution, which would be to add |
I've walked through the behavior that the raw GitHub API performs when I run the Update branch protection command via curl, and this is what I've found: I can set branch protection (requiring status checks) with only a check# Pass in a check (result is successful)
curl -L \
-X PUT \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer <YOUR-TOKEN>" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/OWNER/REPO/branches/BRANCH/protection \
-d '{"required_status_checks":{"strict":true,"checks":[{"context":"continuous-integration/travis-ci","app_id":12345}]},"enforce_admins":true,"required_pull_request_reviews":{"dismissal_restrictions":{"users":["octocat"],"teams":["justice-league"]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count":2,"require_last_push_approval":true,"bypass_pull_request_allowances":{"users":["octocat"],"teams":["justice-league"]}},"restrictions":{"users":["octocat"],"teams":["justice-league"],"apps":["super-ci"]},"required_linear_history":true,"allow_force_pushes":true,"allow_deletions":true,"block_creations":true,"required_conversation_resolution":true,"lock_branch":true,"allow_fork_syncing":true}' I can set branch protection (requiring status checks) with an EMPTY check# Pass in an empty check (result is successful)
curl -L \
-X PUT \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer <YOUR-TOKEN>" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/OWNER/REPO/branches/BRANCH/protection \
-d '{"required_status_checks":{"strict":true,"checks":[]},"enforce_admins":true,"required_pull_request_reviews":{"dismissal_restrictions":{"users":["octocat"],"teams":["justice-league"]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count":2,"require_last_push_approval":true,"bypass_pull_request_allowances":{"users":["octocat"],"teams":["justice-league"]}},"restrictions":{"users":["octocat"],"teams":["justice-league"],"apps":["super-ci"]},"required_linear_history":true,"allow_force_pushes":true,"allow_deletions":true,"block_creations":true,"required_conversation_resolution":true,"lock_branch":true,"allow_fork_syncing":true}' I can set branch protection (requiring status checks) with only a context# Pass in a context (result is successful)
curl -L \
-X PUT \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer <YOUR-TOKEN>" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/OWNER/REPO/branches/BRANCH/protection \
-d '{"required_status_checks":{"strict":true,"contexts":["continuous-integration/travis-ci"]},"enforce_admins":true,"required_pull_request_reviews":{"dismissal_restrictions":{"users":["octocat"],"teams":["justice-league"]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count":2,"require_last_push_approval":true,"bypass_pull_request_allowances":{"users":["octocat"],"teams":["justice-league"]}},"restrictions":{"users":["octocat"],"teams":["justice-league"],"apps":["super-ci"]},"required_linear_history":true,"allow_force_pushes":true,"allow_deletions":true,"block_creations":true,"required_conversation_resolution":true,"lock_branch":true,"allow_fork_syncing":true}' I can set branch protection (requiring status checks) with an EMPTY context# Pass in an empty context (result is successful)
curl -L \
-X PUT \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer <YOUR-TOKEN>" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/OWNER/REPO/branches/BRANCH/protection \
-d '{"required_status_checks":{"strict":true,"contexts":[]},"enforce_admins":true,"required_pull_request_reviews":{"dismissal_restrictions":{"users":["octocat"],"teams":["justice-league"]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count":2,"require_last_push_approval":true,"bypass_pull_request_allowances":{"users":["octocat"],"teams":["justice-league"]}},"restrictions":{"users":["octocat"],"teams":["justice-league"],"apps":["super-ci"]},"required_linear_history":true,"allow_force_pushes":true,"allow_deletions":true,"block_creations":true,"required_conversation_resolution":true,"lock_branch":true,"allow_fork_syncing":true}' I CANNOT set branch protection (requiring status checks) without a check or a context# Pass in a neither a check nor a context
# Result:
# {
# "message": "Invalid request.\n\nNo subschema in \"anyOf\" matched.\nNo subschema in \"oneOf\" matched.\nNot all subschemas of \"allOf\" matched.\nFor 'anyOf/1', {\"strict\"=>true} is not a null.",
# "documentation_url": "https://docs.github.com/rest/branches/branch-protection#update-branch-protection"
# }
curl -L \
-X PUT \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer <YOUR-TOKEN>" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/OWNER/REPO/branches/BRANCH/protection \
-d '{"required_status_checks":{"strict":true},"enforce_admins":true,"required_pull_request_reviews":{"dismissal_restrictions":{"users":["octocat"],"teams":["justice-league"]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count":2,"require_last_push_approval":true,"bypass_pull_request_allowances":{"users":["octocat"],"teams":["justice-league"]}},"restrictions":{"users":["octocat"],"teams":["justice-league"],"apps":["super-ci"]},"required_linear_history":true,"allow_force_pushes":true,"allow_deletions":true,"block_creations":true,"required_conversation_resolution":true,"lock_branch":true,"allow_fork_syncing":true}' I CANNOT set branch protection (requiring status checks) with both a check and a context, regardless of whether they're empty or not# Pass in both a check and a context
# Result:
# {
# "message": "Invalid request.\n\nNo subschema in \"anyOf\" matched.\nMore than one subschema in \"oneOf\" matched.\nNot all subschemas of \"allOf\" matched.\nFor 'anyOf/1', {\"strict\"=>true, \"contexts\"=>[\"continuous-integration/travis-ci\"], \"checks\"=>[{\"context\"=>\"continuous-integration/travis-ci\", \"app_id\"=>12345}]} is not a null.",
# "documentation_url": "https://docs.github.com/rest/branches/branch-protection#update-branch-protection"
# }
curl -L \
-X PUT \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer <YOUR-TOKEN>" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/OWNER/REPO/branches/BRANCH/protection \
-d '{"required_status_checks":{"strict":true,"contexts":["continuous-integration/travis-ci"],"checks":[{"context":"continuous-integration/travis-ci","app_id":12345}]},"enforce_admins":true,"required_pull_request_reviews":{"dismissal_restrictions":{"users":["octocat"],"teams":["justice-league"]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count":2,"require_last_push_approval":true,"bypass_pull_request_allowances":{"users":["octocat"],"teams":["justice-league"]}},"restrictions":{"users":["octocat"],"teams":["justice-league"],"apps":["super-ci"]},"required_linear_history":true,"allow_force_pushes":true,"allow_deletions":true,"block_creations":true,"required_conversation_resolution":true,"lock_branch":true,"allow_fork_syncing":true}' I can set branch protection WITHOUT requiring status checks at all# Do not require status checks at all (result is successful)
curl -L \
-X PUT \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer <YOUR-TOKEN>" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/OWNER/REPO/branches/BRANCH/protection \
-d '{"required_status_checks":null,"enforce_admins":true,"required_pull_request_reviews":{"dismissal_restrictions":{"users":["octocat"],"teams":["justice-league"]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count":2,"require_last_push_approval":true,"bypass_pull_request_allowances":{"users":["octocat"],"teams":["justice-league"]}},"restrictions":{"users":["octocat"],"teams":["justice-league"],"apps":["super-ci"]},"required_linear_history":true,"allow_force_pushes":true,"allow_deletions":true,"block_creations":true,"required_conversation_resolution":true,"lock_branch":true,"allow_fork_syncing":true}' Based on all of this, I've come to the conclusion that GitHub's docs have some typos. The docs say that I believe the Go API should play by the same rules as the GitHub API: I can set branch protection (requiring status checks) with only a check// Pass in a check (result is successful)
var appID int64 = 123456
requiredStatusCheck := github.RequiredStatusCheck{
Context: "helloworld",
AppID: &appID,
}
protectionRequest := &github.ProtectionRequest{
AllowDeletions: github.Bool(false),
AllowForcePushes: github.Bool(false),
AllowForkSyncing: github.Bool(true),
BlockCreations: github.Bool(true),
EnforceAdmins: false,
LockBranch: github.Bool(false),
RequireLinearHistory: github.Bool(false),
RequiredConversationResolution: github.Bool(true),
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{
DismissStaleReviews: true,
RequireCodeOwnerReviews: true,
RequireLastPushApproval: github.Bool(false),
RequiredApprovingReviewCount: 1,
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Checks: []*github.RequiredStatusCheck{&requiredStatusCheck},
Strict: true,
},
}
_, _, err := c.Client.Repositories.UpdateBranchProtection(context.Background(), "OWNER", "REPO", "BRANCH", protectionRequest)
if err != nil {
return err
} I can set branch protection (requiring status checks) with an EMPTY check// Pass in an empty check (result SHOULD BE successful), but isn't
// Result:
// No subschema in "anyOf" matched.
// No subschema in "oneOf" matched.
// Not all subschemas of "allOf" matched.
// For 'anyOf/1', {"strict"=>true} is not a null. []
protectionRequest := &github.ProtectionRequest{
AllowDeletions: github.Bool(false),
AllowForcePushes: github.Bool(false),
AllowForkSyncing: github.Bool(true),
BlockCreations: github.Bool(true),
EnforceAdmins: false,
LockBranch: github.Bool(false),
RequireLinearHistory: github.Bool(false),
RequiredConversationResolution: github.Bool(true),
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{
DismissStaleReviews: true,
RequireCodeOwnerReviews: true,
RequireLastPushApproval: github.Bool(false),
RequiredApprovingReviewCount: 1,
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Checks: []*github.RequiredStatusCheck{},
Strict: true,
},
}
_, _, err := c.Client.Repositories.UpdateBranchProtection(context.Background(), "OWNER", "REPO", "BRANCH", protectionRequest)
if err != nil {
return err
} I can set branch protection (requiring status checks) with only a context// Pass in a context (result is successful)
protectionRequest := &github.ProtectionRequest{
AllowDeletions: github.Bool(false),
AllowForcePushes: github.Bool(false),
AllowForkSyncing: github.Bool(true),
BlockCreations: github.Bool(true),
EnforceAdmins: false,
LockBranch: github.Bool(false),
RequireLinearHistory: github.Bool(false),
RequiredConversationResolution: github.Bool(true),
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{
DismissStaleReviews: true,
RequireCodeOwnerReviews: true,
RequireLastPushApproval: github.Bool(false),
RequiredApprovingReviewCount: 1,
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Contexts: []string{"helloworld"},
Strict: true,
},
}
_, _, err := c.Client.Repositories.UpdateBranchProtection(context.Background(), "OWNER", "REPO", "BRANCH", protectionRequest)
if err != nil {
return err
} I can set branch protection (requiring status checks) with an EMPTY context// Do not require status checks at all (result SHOULD BE successful), but isn't
// Result:
// No subschema in "anyOf" matched.
// No subschema in "oneOf" matched.
// Not all subschemas of "allOf" matched.
// For 'anyOf/1', {"strict"=>true} is not a null. []
protectionRequest := &github.ProtectionRequest{
AllowDeletions: github.Bool(false),
AllowForcePushes: github.Bool(false),
AllowForkSyncing: github.Bool(true),
BlockCreations: github.Bool(true),
EnforceAdmins: false,
LockBranch: github.Bool(false),
RequireLinearHistory: github.Bool(false),
RequiredConversationResolution: github.Bool(true),
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{
DismissStaleReviews: true,
RequireCodeOwnerReviews: true,
RequireLastPushApproval: github.Bool(false),
RequiredApprovingReviewCount: 1,
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Contexts: []string{},
Strict: true,
},
}
_, _, err := c.Client.Repositories.UpdateBranchProtection(context.Background(), "OWNER", "REPO", "BRANCH", protectionRequest)
if err != nil {
return err
} I CANNOT set branch protection (requiring status checks) without a check or a context// Pass in a neither a check nor a context
// Result:
// No subschema in "anyOf" matched.
// No subschema in "oneOf" matched.
// Not all subschemas of "allOf" matched.
// For 'anyOf/1', {"strict"=>true} is not a null. []
protectionRequest := &github.ProtectionRequest{
AllowDeletions: github.Bool(false),
AllowForcePushes: github.Bool(false),
AllowForkSyncing: github.Bool(true),
BlockCreations: github.Bool(true),
EnforceAdmins: false,
LockBranch: github.Bool(false),
RequireLinearHistory: github.Bool(false),
RequiredConversationResolution: github.Bool(true),
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{
DismissStaleReviews: true,
RequireCodeOwnerReviews: true,
RequireLastPushApproval: github.Bool(false),
RequiredApprovingReviewCount: 1,
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Strict: true,
},
}
_, _, err := c.Client.Repositories.UpdateBranchProtection(context.Background(), "OWNER", "REPO", "BRANCH", protectionRequest)
if err != nil {
return err
} I CANNOT set branch protection (requiring status checks) with both a check and a context, regardless of whether they're empty or not// Pass in both a check and a context
// Result:
// No subschema in "anyOf" matched.
// More than one subschema in "oneOf" matched.
// Not all subschemas of "allOf" matched.
// For 'anyOf/1', {"strict"=>true, "contexts"=>["hello-world"], "checks"=>[{"context"=>"helloworld", "app_id"=>123456}]} is not a null. []
var appID int64 = 123456
requiredStatusCheck := github.RequiredStatusCheck{
Context: "helloworld",
AppID: &appID,
}
protectionRequest := &github.ProtectionRequest{
AllowDeletions: github.Bool(false),
AllowForcePushes: github.Bool(false),
AllowForkSyncing: github.Bool(true),
BlockCreations: github.Bool(true),
EnforceAdmins: false,
LockBranch: github.Bool(false),
RequireLinearHistory: github.Bool(false),
RequiredConversationResolution: github.Bool(true),
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{
DismissStaleReviews: true,
RequireCodeOwnerReviews: true,
RequireLastPushApproval: github.Bool(false),
RequiredApprovingReviewCount: 1,
},
RequiredStatusChecks: &github.RequiredStatusChecks{
Checks: []*github.RequiredStatusCheck{&requiredStatusCheck},
Contexts: []string{"hello-world"},
Strict: true,
},
}
_, _, err := c.Client.Repositories.UpdateBranchProtection(context.Background(), "OWNER", "REPO", "BRANCH", protectionRequest)
if err != nil {
return err
} I can set branch protection WITHOUT requiring status checks at all// Do not require status checks at all (result is successful)
protectionRequest := &github.ProtectionRequest{
AllowDeletions: github.Bool(false),
AllowForcePushes: github.Bool(false),
AllowForkSyncing: github.Bool(true),
BlockCreations: github.Bool(true),
EnforceAdmins: false,
LockBranch: github.Bool(false),
RequireLinearHistory: github.Bool(false),
RequiredConversationResolution: github.Bool(true),
RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{
DismissStaleReviews: true,
RequireCodeOwnerReviews: true,
RequireLastPushApproval: github.Bool(false),
RequiredApprovingReviewCount: 1,
},
RequiredStatusChecks: nil,
}
_, _, err := c.Client.Repositories.UpdateBranchProtection(context.Background(), "OWNER", "REPO", "BRANCH", protectionRequest)
if err != nil {
return err
} NOTE: All of these blocks and shown behavior are from v57 of the API, with There are two cases I've outlined above where the existing Go API doesn't align with the GitHub API, and that's where the In an ideal world, there'd be more logic determining whether to omit empty or not, AKA if user passes I would be open to making this PR, if we agree this is the right approach. One last note. Need a workaround for setting empty I managed to come up with two potential workarounds that I'll show below. Neither are ideal, but workarounds usually aren't 😄. Use API v56, as
|
If contexts are deprecated, why not just drop support for them in this library? People who still want to use them can use an older version of this library as well... |
We try to deprecate before removing, in general, but you do have a good point. It is just a courtesy to the developers, really. |
@gmlewis I believe if we turn them both into pointers, then it will properly send type RequiredStatusChecks struct {
// Require branches to be up to date before merging. (Required.)
Strict bool `json:"strict"`
// The list of status checks to require in order to merge into this
// branch. An empty slice is valid. (Deprecated. Note: only one of
// Contexts/Checks can be populated, but at least one must be populated).
Contexts *[]string `json:"contexts,omitempty"`
// The list of status checks to require in order to merge into this
// branch. An empty slice is valid.
Checks *[]*RequiredStatusCheck `json:"checks,omitempty"`
ContextsURL *string `json:"contexts_url,omitempty"`
URL *string `json:"url,omitempty"`
} Would this have any negative ramifications for other parts of the code? |
@gmlewis @luisdavim I made the change to switch both Pull request link: #3070 |
We found an issue with
Branch Protection fields
that were added as part of 4caa1d6The problem is with
it needs the
omitempty
tag added to it.A fix is on the way
The text was updated successfully, but these errors were encountered: