-
Notifications
You must be signed in to change notification settings - Fork 56
feat: gitlab-11.2-group-api-improvements #39
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
feat: gitlab-11.2-group-api-improvements #39
Conversation
- use the new gitlab 11.2 `min_access_level` parameter in the groups api: publish rights are now configurable based on the access level of the user to the groups, it doesn't require owner permissions - add new configuration parameters for `access` and `publish` access levels
Great! I guess we need the following before we merge:
- compatible with <11.2 (via API call or
legacy_mode: truefor old versions < 11.2) - remove access cache, so just one instead of two api calls to gitlab
- create a upstream issue for access and publish group
Quick question: if we are going to add the legacy flag in the configuration, which will be disabled by default, I guess we can also set the default publish access level to $maintainer? At least it feels more natural to me. When legacy_mode: true, the value applied will be $owner anyway, there's no other option pre-11.2.
Good idea @dlouzan , fully agree!
- remove `access` access level configuration parameter since it cannot be used with the current plugins api - support gitlab 9.0+ with a new `legacy_mode` configuration flag; when not in legacy mode, gitlab 11.2+ is required - improved README, document all configuration options
c6be5e7 to
68068bf
Compare
Just pushed the latest changes:
- remove
accessaccess level configuration parameter since it cannot be used with the current plugins api - support gitlab 9.0+ with a new
legacy_modeconfiguration flag; when not in legacy mode, gitlab 11.2+ is required - improved README, document all configuration options
README.md
Outdated
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 write false or not defined here
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.
It's indicated below in the configuration options list that legacy_mode: false is the default, but I will make it also explicit here.
conf/localhost.yaml
Outdated
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.
this file is nearly the same as conf/docker.yml , why do we need this?
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.
Ooops, that's for my local testing, shouldn't be there
- remove spurious localhost tests configuration file - misc corrections in README
89bdcbf to
207a490
Compare
Done, I'm adding commits on top of the original PR commit. Feel free to squash when accepting.
Great work!
min_access_levelparameter in the groups api:publish rights are now configurable based on the access level of the
user to the groups, it doesn't require owner permissions
accessandpublishaccesslevels
Fixes #5