-
Notifications
You must be signed in to change notification settings - Fork 706
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
Add an option to create a namespace #1489
Conversation
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.
Excellent stuff Andres. That's much clearer and explicit. Thanks!
@@ -20,7 +20,8 @@ class PermissionsErrorPage extends React.Component<IPermissionsErrorPage> { | |||
<UnexpectedErrorAlert | |||
title={ | |||
<span> | |||
You don't have sufficient permissions to {action} in {namespaceText(namespace)} | |||
You don't have sufficient permissions to {action}{" "} | |||
{namespace && <>in {namespaceText(namespace)}</>} |
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.
Much clearer :)
if (to === pathname) { | ||
setNamespace(ns); | ||
} else { | ||
setNamespace(ns); |
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 change just means that setNamespace
is also called when the path changes, rather than only when it didn't change? Why was it only calling setNamespace
when it hadn't changed in the first place?
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.
Yes, the logic was: If we are in a route that doesn't contain the namespace (e.g. /catalog) just call setNamespace
. In other case, it was relying that a change in the route would trigger the setNamespace
action by someone else (I assume). But the second was never happening and setNamespace
was not being called.
options: [ | ||
{ label: "All Namespaces", value: "_all" }, | ||
{ label: defaultProps.defaultNamespace, value: defaultProps.defaultNamespace }, | ||
{ label: "Create New", value: "new" }, |
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 wondering if we need to show the option to "Create New" namespace if we know the user cannot do this. One simple, but not perfect, way is that we could only show the option to create a new namespace when there are more than 2 options in the selector already. It's not perfect as it only indicates the user can read namespaces, but is trivial to implement (EDIT: Actually, forget that, as we can assign RBAC to write namespaces without read, I assume). Not sure if it's worth considering returning this info when getting namespaces, but either way, it'd be another PR. See what you think.
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.
IMO hiding things may lead to a worse UX. If I am an unprivileged user and I want to create a namespace, I get a clear error that I don't have enough permissions to do so. If we hide the option it's not clear if I cannot do it or if it's the tool the one that doesn't support 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.
Yep, good point.
if (value.value === "new") { | ||
this.setState({ modalIsOpen: true }); | ||
return; | ||
} |
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.
Out of interest, what happens if I create a new namespace named "new", and then try to select it? :P (depending on the answer, you might need a token which is not a valid namespace).
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.
good catch!
I always wondered why the All Namespaces choice used _all
(it does because _all
is an invalid namespace name).
Very cool feature and very beneficial when KubeAps is use with Helm3. Thanks @andresmgot ! |
Description of the change
Follow up of #1483
Add an item to the namespace selector to create a namespace. This opens a modal to introduce the new namespace:
In the case of error, it's shown to the user:
Applicable issues
This is not fixing #1364 yet because it only allows you to create a namespace but you are still able to select a non-existing namespace and try to create an application there (which returns a wrong error).