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
Change API to be able to assign roles to namespaces #607
Change API to be able to assign roles to namespaces #607
Changes from all commits
dde73cc
886cb37
0e76b63
3bfba54
ed2dafd
9d2da3f
f853a64
bd3b9de
d369a1a
2a82e28
ffb358b
0b82072
efdc6c1
4dce717
30dbba1
94f5804
f0a302f
ac6c117
169bef7
3c998e6
090a857
f0c75f6
09e6708
1016952
a681b69
58b959a
70c20f3
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.
Can we please have more descriptive variable names here? Readability is part of maintainability.
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.
In general, I agree. But not here. I deliberately use short names so that queries don't get split up over multiple lines by the linter, which would be hard to read. All these DB functions are very small, so there's no concern of this clashing with something else causing confusion. I couldn't think of a short name for NamespaceRoleMapping other than using the first letters.
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.
As mentioned above, these names don't make it clear which namespace has permissions to which.
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.
Addressed in another comment above.
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.
Added this to print an exception to the terminal when an invalid
role_mappings_version
value is used. Previously, nothing was printed except "Invalid server error".