-
Notifications
You must be signed in to change notification settings - Fork 48
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
Support aliases and groups in allowed_* config options for Koji #2320
Support aliases and groups in allowed_* config options for Koji #2320
Conversation
Build failed. ✔️ pre-commit SUCCESS in 2m 00s |
033fee3
to
bf549f0
Compare
Build failed. ❌ pre-commit FAILURE in 1m 57s |
bf549f0
to
436140e
Compare
Build succeeded. ✔️ pre-commit SUCCESS in 2m 06s |
Introduce aliases `all_admins` and `all_committers` and also support groups in allowed_pr_authors and allowed_committers config options Fixes packit/packit#2088 Requires packit/ogr#834
436140e
to
cea6451
Compare
all_admins = "all_admins" | ||
all_committers = "all_committers" |
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'm just wondering about the all_
prefix. An alias is technically a project-specific virtual group, and if we use a @
symbol for groups, we could use something similar for the aliases.
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.
personally I like the original proposal, having it like the current implementation, but no strong opinion. @packit/the-packit-team anyone has any preferences?
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 have strong opinions either.
But I wouldn't use the @ symbol. I agree that we have created a group, but it is a "packit" group and it will not show up in the fedoraproject UI; for this reason I think it is misleading to use @
We can have another symbol but it will not "immediately suggest" to the user the "group idea", so I don't know if it is worth having it?
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 just think that using a symbol is a little more consistent and gives the feeling that it's some special identifier, on the other hand all_admins
is also pretty self-explanatory:
allowed_pr_authors:
- $admins
- @packager
- packit-stg
allowed_pr_authors:
- all_admins
- @packager
- packit-stg
I'm fine with both.
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.
If we used $
, we could call it a variable rather than alias: $admins expands to all users with admin access to the project
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.
We already have the aliases for targets
and these also do not use any special symbols. So if we would like to be consistent, I would go with the original idea.
Build succeeded. ✔️ pre-commit SUCCESS in 1m 59s |
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 2m 18s |
f3ec1ad
into
packit:main
Introduce aliases
all_admins
andall_committers
and also support groups in allowed_pr_authors and allowed_committers config optionsFixes packit/packit#2088
Requires packit/ogr#834
TODO:
packit/packit.dev
. (Document support for groups and aliases in koji allowed_* options packit.dev#814)RELEASE NOTES BEGIN
allowed_pr_authors
andallowed_committers
now allow specifying groups and also aliasesall_admins
andall_committers
(corresponding to the access to the repository).RELEASE NOTES END