-
Notifications
You must be signed in to change notification settings - Fork 959
Add support to Groups APIs for DefaultBranchDefaults #1971
Add support to Groups APIs for DefaultBranchDefaults #1971
Conversation
@svanharmelen - This should be ready for review. Let me know what you think about the naming for |
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.
Have a few suggestion, but other then that I'm good with this...
@@ -90,6 +95,16 @@ type Group struct { | |||
|
|||
// Deprecated: Use EmailsEnabled instead | |||
EmailsDisabled bool `json:"emails_disabled"` | |||
|
|||
// Deprecated: Use DefaultBranchProtectionDefaults instead | |||
DefaultBranchProtection int `json:"default_branch_protection"` |
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 don't think this is deprecated looking at the docs.
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.
@svanharmelen - Hm, some of the conversations I've had with the GitLab employees certainly seem to insinuate it is, so I never thought about the fact that it isn't documented that way 🤔
I'll talk with them on Monday and confirm it's status, then I'll either update the documentation or update this PR. Thanks for the great catch!
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.
https://gitlab.com/gitlab-org/gitlab/-/issues/462164#note_1998263497 for the question to GitLab
groups.go
Outdated
DefaultBranchProtection int `json:"default_branch_protection"` | ||
} | ||
|
||
// GroupAccessLevel represents an entry in a Group defining the access level for default protections |
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.
// GroupAccessLevel represents an entry in a Group defining the access level for default protections | |
// GroupAccessLevel represents default branch protection defaults access levels. |
Still not great, but close enough.
groups.go
Outdated
|
||
// GroupAccessLevel represents an entry in a Group defining the access level for default protections | ||
// | ||
// GitLab API docs: https://docs.gitlab.com/ee/api/groups.html#options-for-default_branch_protection_defaults |
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.
// GitLab API docs: https://docs.gitlab.com/ee/api/groups.html#options-for-default_branch_protection_defaults | |
// GitLab API docs: | |
// https://docs.gitlab.com/ee/api/groups.html#options-for-default_branch_protection_defaults |
|
||
// Deprecated: Use EmailsEnabled instead | ||
EmailsDisabled *bool `url:"emails_disabled,omitempty" json:"emails_disabled,omitempty"` | ||
|
||
// Deprecated: User DefaultBranchProtectionDefaults instead | ||
DefaultBranchProtection *int `url:"default_branch_protection,omitempty" json:"default_branch_protection,omitempty"` |
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 for this one of course (re the deprecation).
groups.go
Outdated
DefaultBranchProtection *int `url:"default_branch_protection,omitempty" json:"default_branch_protection,omitempty"` | ||
} | ||
|
||
// DefaultBranchProtectionDefaultsOptions represents the available options for using default_branch_protection_defaults in CreateGroup() or UpdateGroup() |
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.
Please keep comments below 80 chars when possible...
// DefaultBranchProtectionDefaultsOptions represents the available options for using default_branch_protection_defaults in CreateGroup() or UpdateGroup() | |
// DefaultBranchProtectionDefaultsOptions represents the available options for | |
// using default_branch_protection_defaults in CreateGroup() or UpdateGroup() |
groups.go
Outdated
|
||
// DefaultBranchProtectionDefaultsOptions represents the available options for using default_branch_protection_defaults in CreateGroup() or UpdateGroup() | ||
// | ||
// GitLab API docs: https://docs.gitlab.com/ee/api/groups.html#options-for-default_branch_protection_defaults |
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.
// GitLab API docs: https://docs.gitlab.com/ee/api/groups.html#options-for-default_branch_protection_defaults | |
// GitLab API docs: | |
// https://docs.gitlab.com/ee/api/groups.html#options-for-default_branch_protection_defaults |
groups.go
Outdated
AllowedToPush []GroupAccessLevel `url:"allowed_to_push,omitempty" json:"allowed_to_push,omitempty"` | ||
AllowForcePush bool `url:"allow_force_push,omitempty" json:"allow_force_push,omitempty"` | ||
AllowedToMerge []GroupAccessLevel `url:"allowed_to_merge.omitempty" json:"allowed_to_merge.omitempty"` | ||
DeveloperCanInitialPush bool `url:"developer_can_initial_push,omitempty" json:"developer_can_initial_push,omitempty"` |
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.
However weird it may be, these should all be pointers as this is an options struct...
AllowedToPush []GroupAccessLevel `url:"allowed_to_push,omitempty" json:"allowed_to_push,omitempty"` | |
AllowForcePush bool `url:"allow_force_push,omitempty" json:"allow_force_push,omitempty"` | |
AllowedToMerge []GroupAccessLevel `url:"allowed_to_merge.omitempty" json:"allowed_to_merge.omitempty"` | |
DeveloperCanInitialPush bool `url:"developer_can_initial_push,omitempty" json:"developer_can_initial_push,omitempty"` | |
AllowedToPush *[]*GroupAccessLevel `url:"allowed_to_push,omitempty" json:"allowed_to_push,omitempty"` | |
AllowForcePush *bool `url:"allow_force_push,omitempty" json:"allow_force_push,omitempty"` | |
AllowedToMerge *[]*GroupAccessLevel `url:"allowed_to_merge.omitempty" json:"allowed_to_merge.omitempty"` | |
DeveloperCanInitialPush *bool `url:"developer_can_initial_push,omitempty" json:"developer_can_initial_push,omitempty"` |
|
||
// Deprecated: Use EmailsEnabled instead | ||
EmailsDisabled *bool `url:"emails_disabled,omitempty" json:"emails_disabled,omitempty"` | ||
|
||
// Deprecated: Use DefaultBranchProtectionDefaults instead | ||
DefaultBranchProtection *int `url:"default_branch_protection,omitempty" json:"default_branch_protection,omitempty"` |
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...
Everything other than the deprecation notices has been fixed; will update Deprecation noticed in either direction on Monday and let you know! |
@svanharmelen - Documentation is updated and now shows the fields as Deprecated, so having them labelled as deprecated is correct: https://docs.gitlab.com/ee/api/groups.html#new-group Reference from conversations with GitLab: https://gitlab.com/gitlab-org/gitlab/-/issues/462164#note_1999148308 |
This Pull Request adds support for the new
default_branch_protection_defaults
attribute to the Group APIs. These were added in GitLab 17.0, replacing thedefault_branch_protection
integer, so that attribute is now marked as deprecated as well.Documentation: https://docs.gitlab.com/ee/api/groups.html#options-for-default_branch_protection_defaults
Note: The structure of the API requests is weird. Even though
allowed_to_push
andallowed_to_merge
only expect 1 access level, they do in fact require an array of objects with "access_level" being the key.Here is an example request and responss to show the API fingerprint, since I don't think it's exactly clear from the documentation how it's supposed to work 😆 :