-
Notifications
You must be signed in to change notification settings - Fork 91
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
Update allowed admin groups #2429
Conversation
0b53700
to
35146ff
Compare
35146ff
to
81d51d6
Compare
This needs to be tested; based on the changes, I don't foresee any problems. but its good to double check |
We tested this on Quansight deployment and a client deployment. This is a fix for the consequence of: 6a83ada This could be soon removed, when we have roles syncing and proper permissions model, which is WIP. This PR is just be bring back the old behaviour meanwhile. |
allowed_groups = ["/analyst", "/developer", "/admin", "jupyterhub_admin", "jupyterhub_developer"] | ||
admin_groups = ["/admin", "jupyterhub_admin"] |
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.
Hi @aktech wouldn't this create conda-store namespaces and EFS share files for those then when a users receives such group?
@krassowski, if you have some time today, could you just double-check that those permissions are enough? |
As this is a release blocker I will go ahead an merge this PR, and any extra thoughts can be done in a separate PR after the first RC |
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.
LGTM
Reference Issues or PRs
Fixes #2436
Due to a recent change, adding
jupyterhub_admin
role to a user doesn't makes it admin for JupyterHub, this PR fixes that.What does this implement/fix?
Put a
x
in the boxes that applyTesting
Any other comments?