Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Refactor shared group mounting using RBAC #2593
Refactor shared group mounting using RBAC #2593
Changes from 2 commits
9e38ab1
ff74a0d
5ca8dbc
6a8e000
bb7a509
fcbd4b6
8025f0c
afca632
c5f651c
7bc8a04
b1336f8
a100a44
8f05abb
7dd72cd
3324d30
5713783
742f7fe
c955e6a
fc349d9
d2cbb94
53cfcd3
655b743
6f0ad66
e448f17
c1e0a95
0c5d9aa
3ce8e88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there any reason we need to loop through groups to create a list, then loop through that list to create a list of volume mounts and a list of commands and a list of init containers??
Couldn't we just generate the list of volume mounts, commands and init containers in the original loop and just use them later? That would be more readable and reduce the loops from 4 to 1
It may even be worth extracting that loop into a seperate function that takes a list of groups and group roles in and returns the 3 lists that you need further in this function.
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 complelty agree; though most of this was already here.If possible, I would prefer to include enhancements in a separate PR. No strong opinions, if you also consider this would suit this PR I can change that as well 😄
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 is going to cause problem in jhub-apps: https://github.com/nebari-dev/jhub-apps/blob/657f7c871003e5f678d865b2b4aad1845e3b5bf7/jhub_apps/service/utils.py#L53
Since that's a downstream problem, it's not blocking this but I would need to think of an alternative approach in jhub-apps to get the spawner with
authenticator
.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 can add the extra information to the auth_state object (probably), this way you don't need to customize the above mock spawner, and we will not need to rely on
spawner.authenticator
here (which I think its messy).Though, the reason I didn't go for it was that I was skeptical of changing it directly as
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 will have a try on that, and check how it goes
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'll check if there is a less hacky way to do this on the jhub-apps.
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.
Actually there isn't a non-hacky way to do this in jhub-apps. In native JupyterHub the spawner object is magically available due to the huge initialisation done at the startup.
Ref:
We do need to set the groups via
auth_state
and in-fact I think that's even cleaner as all the roles and scopes parsing happens in the authenticator and other components like spawner just consumes the output viaauth_state
.