Skip to content
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

Add project setting ci_pipeline_variables_minimum_override_role #2057

Merged
merged 5 commits into from
Nov 12, 2024

Conversation

vindvaki
Copy link
Contributor

@vindvaki vindvaki commented Nov 6, 2024

This is a companion setting to restrict_user_defined_variables, allowing more granular control over who can set job variables and pipeline variables in a project. Currently, the setting restrict_user_defined_variables acts as a toggle for the restriction set by ci_pipeline_variables_minimum_override_role, but it is likely that restrict_user_defined_variable will be deprecated in favor of just ci_pipeline_variables_minimum_override_role in the future.

See https://docs.gitlab.com/ee/ci/variables/#restrict-who-can-override-variables

This is a companion setting to `restrict_user_defined_variables`,
allowing more granular control over who can set job variables and
pipeline variables in a project. Currently, the setting
`restrict_user_defined_variables` acts as a toggle for the restriction
set by `ci_pipeline_variables_minimum_override_role`, but it is likely
that `restrict_user_defined_variable` will be deprecated in favor of
just `ci_pipeline_variables_minimum_override_role` in the future.

See https://docs.gitlab.com/ee/ci/variables/#restrict-who-can-override-variables
projects.go Outdated
@@ -157,6 +157,7 @@ type Project struct {
MergePipelinesEnabled bool `json:"merge_pipelines_enabled"`
MergeTrainsEnabled bool `json:"merge_trains_enabled"`
RestrictUserDefinedVariables bool `json:"restrict_user_defined_variables"`
CIPipelineVariablesMinimumOverrideRole AccessControlValue `json:"ci_pipeline_variables_minimum_override_role"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose AccessControlValue to match existing code, but for what it's worth, the AccessControlValue type seems to have been originally intended for something completely different. Over time, it seems to have morphed into more of a catch-all string type for different access control values 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vindvaki - I agree, I'm not sure AccessControlValue is the right type here. Since the API seems like it's accepting a string value for the Access Level instead of the integer value (which is usually defined using the AccessLevelValue type), I think we should probably create a new type for it in types.go, especially since no_one_allowed seems to deviate from the normal no_one that's used elsewhere.

would you mind doing that? Otherwise this LGTM!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RicePatrick sure, I've created a new type in fc7656e. Let me know what you think 🏓

Copy link
Collaborator

@RicePatrick RicePatrick Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vindvaki - I apologize for nitpicking, but would you mind adding a comment on the new type to match the values of the others within the file? I tried to push a commit for you, but I don't have permissions to do so :)

I'd suggest something like this:

// CIPipelineVariablesMinimumOverrideRoleValue represents an access control
// value used for managing access to the CI Pipeline Variable Override feature.
//
// GitLab API docs: https://docs.gitlab.com/ee/api/projects.html
type CIPipelineVariablesMinimumOverrideRoleValue = string

// List of available CIPipelineVariablesMinimumOverrideRoleValue levels.
//
// GitLab API docs: https://docs.gitlab.com/ee/api/projects.html
const (
	CIPipelineVariables_NoOneAllowedRole CIPipelineVariablesMinimumOverrideRoleValue = "no_one_allowed"
	CiPipelineVariables_OwnerRole        CIPipelineVariablesMinimumOverrideRoleValue = "owner"
	CiPipelineVariables_MaintainerRole   CIPipelineVariablesMinimumOverrideRoleValue = "maintainer"
	CIPipelineVariables_DeveloperRole    CIPipelineVariablesMinimumOverrideRoleValue = "developer"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RicePatrick thanks, done 🏓

@vindvaki
Copy link
Contributor Author

vindvaki commented Nov 7, 2024

@timofurrer @RicePatrick looks like you are the new maintainers? Could either of you please review? Thanks!

@RicePatrick
Copy link
Collaborator

@vindvaki - Yep, I'll probably get to it tomorrow; thanks for the ping!

@RicePatrick
Copy link
Collaborator

Approved the GitHub workflows, and one last nitpick then this LGTM!

Copy link
Collaborator

@timofurrer timofurrer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vindvaki sorry for another nitpick. I'm trying to apply myself and merge.

types.go Outdated Show resolved Hide resolved
projects_test.go Outdated Show resolved Hide resolved
@timofurrer timofurrer self-requested a review November 12, 2024 09:45
@timofurrer
Copy link
Collaborator

@vindvaki @RicePatrick I'm going to merge here. Thanks Hordur! I assume you need a release?

@timofurrer timofurrer merged commit 8eaea58 into xanzy:main Nov 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants