Return a valid error when using non-existing namespaces #1491

andresmgot · 2020-01-30T16:16:15Z

Description of the change

Follow up of #1489 to fix #1364

When selecting a namespace that doesn't exists, shows an error so the namespace is created in advance:

Applicable issues

fixes Newly creates namespace gone after namespace switch #1364

Additional information

cc/ @fabianschwarzfritz

absoludity · 2020-01-31T00:34:54Z

I'm not sure this is the best UX to solve this, but you may have already worked out why the following isn't possible. My main concern is that we're making the helm 3 UX more complicated so that we can support the Helm 2 case, and I wonder if there's a better way forward (in particular, carrying over the error throughout the installation process via the red check).

So, I think the following are the ideals for each case, Helm 2 and Helm 3:

Helm 2: I'm allowed to select a non-existent namespace, because helm 2 will try to create it on install automatically. At that point it'll either just work (creating the namespace) or perhaps display an error if we don't have RBAC (or does tiller do the namespace, create - haven't checked). So the bug can be solved here by checking, when installing, whether the release namespace is not yet in the list of namespaces in state, and if not, reload then.

Helm 3: Do not allow selecting a non-existent namespace, giving feedback straight away that it must be created.

And my questions are:

Why would we not just use the above Helm 3 behaviour always: yes, it means that even helm 2 users will need to create the namespace first if they select a non-existent one (rather than having helm 2 create it automatically on install), but I don't think this is a bad thing, and results in a much more consistent UX (and effectively backports the fix in helm 3 to helm 2 for Kubeapps),
If we really wanted to allow the existing behaviour in helm 2, we could allow the above two ideals depending on whether we were using helm 2 or helm 3 (which could be accessible via config). ie. if and only if we are using helm 3, then display an error directly when entering a namespace that doesn't exist.

Let me know if you've already thought through and decided against both of these.

Aside: There have been a number of times when I've gone and done something only to have you point out a better way or why it's not a good idea - maybe we should have quick informal pre-implementation discussion on the standup when we've a plan for items like this - it always helps to bounce/brainstorm ideas, etc.

andresmgot · 2020-01-31T09:37:26Z

Why would we not just use the above Helm 3 behaviour always: yes, it means that even helm 2 users will need to create the namespace first if they select a non-existent one (rather than having helm 2 create it automatically on install), but I don't think this is a bad thing, and results in a much more consistent UX (and effectively backports the fix in helm 3 to helm 2 for Kubeapps),

I didn't want to add a regression in the UX for Helm 2 but it's true that if it's hurting the UX in Helm 3 it may be worth.

If we really wanted to allow the existing behaviour in helm 2, we could allow the above two ideals depending on whether we were using helm 2 or helm 3 (which could be accessible via config). ie. if and only if we are using helm 3, then display an error directly when entering a namespace that doesn't exist.

I don't think this is worth the effort since creating namespaces is trivial

Aside: There have been a number of times when I've gone and done something only to have you point out a better way or why it's not a good idea - maybe we should have quick informal pre-implementation discussion on the standup when we've a plan for items like this - it always helps to bounce/brainstorm ideas, etc.

I agree but lately there has not been a lot of time to discuss these kind of things (and I have started working on these for two reasons: it's not critical and I got blocked on the rest of things I was working on). In any case, I don't think doing these kind of quick PRs is a waste of time (even if they get rejected): it's not a common thing and in any case we explore different ideas without a lot of work.

andresmgot · 2020-01-31T11:54:56Z

Take a look again @absoludity. Now having the namespace is mandatory but I have left the warning icon because it gives extra information, let me know what you think.

absoludity · 2020-02-02T23:40:20Z

I didn't want to add a regression in the UX for Helm 2 but it's true that if it's hurting the UX in Helm 3 it may be worth.

I don't think it's a regression, more like porting a fix back to Helm 2 (ie. ensuring the namespace is created explicitly).

I don't think this is worth the effort since creating namespaces is trivial

+100

I agree but lately there has not been a lot of time to discuss these kind of things (and I have started working on these for two reasons: it's not critical and I got blocked on the rest of things I was working on).

Just let me know if you are ever blocked on something I can unblock - I'm more than happy to jump on and unblock... keen to make time so that neither of us is blocked. +1 to picking up non-critical things when the situation does happen though.

In any case, I don't think doing these kind of quick PRs is a waste of time (even if they get rejected): it's not a common thing and in any case we explore different ideas without a lot of work.

Great, as long as we are intentional about avoiding the sunk cost fallacy (https://yourbias.is/the-sunk-cost-fallacy )

absoludity

Take a look again @absoludity. Now having the namespace is mandatory but I have left the warning icon because it gives extra information, let me know what you think.

Thanks. AFAICT from playing with the frontend with this change, the warning icon with its tool-tip of extra info is only required because we're not displaying that extra info in the actual ErrorBoundary compontent? My preference would be that the extra info is visible directly without requiring the warning+tool-tip.

The error state that you end up in is a little odd too: I can select Catalog, or Configuration->App Repositories, and go to the new route but without any change in the error state (as expected, it's just odd). Ideally we would not let people select a namespace to which they don't have access, but that's a different issue :)

+1, just see what you think.

absoludity · 2020-02-02T23:48:35Z

dashboard/src/actions/namespace.test.tsx

+    expect(store.getActions()).toEqual(expectedActions);
+  });
+
+  it("dispatches errorNamespace if error creating a namespace", async () => {


This is meant to be "if error getting a namespace" I think? (test and actual change are for requestNamespace)

absoludity · 2020-02-03T00:05:34Z

dashboard/src/reducers/namespace.test.ts

@@ -42,13 +43,22 @@ describe("namespaceReducer", () => {
  context("when ERROR_NAMESPACE", () => {
    const err = new Error("Bang!");

-    it("leaves namespaces intact and sets error", () => {
+    it("when listing leaves namespaces intact and but ignores the error", () => {


FWIW, I think this inconsistency will go away if and when we're able to list the namespaces to which the user has access.

absoludity · 2020-02-03T00:08:08Z

dashboard/src/reducers/namespace.ts

+      if (!state.namespaces.includes(action.payload.metadata.name)) {
+        return {
+          ...state,
+          namespaces: state.namespaces.concat(action.payload.metadata.name),


Not sure whether the k8s API returns the namespace list ordered, but we probably want to ensure the same, so that after creating a namespace, the order of the list is the same?

absoludity · 2020-02-03T00:09:28Z

dashboard/src/reducers/namespace.ts

    case getType(actions.namespace.receiveNamespaces):
      return { ...state, namespaces: action.payload };
    case getType(actions.namespace.setNamespace):
      return { ...state, current: action.payload, error: undefined };
    case getType(actions.namespace.errorNamespaces):
+      // Ignore error listing namespaces since those are expected


Does it depend on the error? We expect a 4xx if the user can't get namespaces, but we don't expect other errors?

not really, at least not at this moment, we are just ignoring any error. I know that if we receive a 5xx we should probably do something but that would mean that the k8s API is not responding which the user would notice in some other way? In any case, this wouldn't be a regression.

andresmgot · 2020-02-03T10:06:25Z

Thanks. AFAICT from playing with the frontend with this change, the warning icon with its tool-tip of extra info is only required because we're not displaying that extra info in the actual ErrorBoundary compontent? My preference would be that the extra info is visible directly without requiring the warning+tool-tip.

Yep, if you are not a fan of the tool-tip I think I can move the custom error info to the main error body.

The error state that you end up in is a little odd too: I can select Catalog, or Configuration->App Repositories, and go to the new route but without any change in the error state (as expected, it's just odd). Ideally we would not let people select a namespace to which they don't have access, but that's a different issue :)

There are two problems here that lead to that odd behavior:

The way of changing the namespace is typing in the namespace selector. It would be weird to automatically change back the namespace to the previous one if the name is not valid.
Since the Header and the body don't have a parent-child relation, the only way that the Header can affect the body is using the Redux state. We could revert the namespace change and clean the error when changing the location (a URL change) but that may be weird too?

absoludity · 2020-02-03T12:31:57Z

Great, thanks for the updates Andres.

There are two problems here that lead to that odd behavior:

* The way of changing the namespace is typing in the namespace selector. It would be weird to automatically change back the namespace to the previous one if the name is not valid.

Yep, definitely.

* Since the Header and the body don't have a parent-child relation, the only way that the Header can affect the body is using the Redux state. We could revert the namespace change and clean the error when changing the location (a URL change) but that may be weird too?

Yeah - I think the best solution longer term will be to present users only with the namespaces to which they have access and avoid this entirely. But for now, this is great.

Still +1, Thanks :)

Return a valid error when using non-existing namespaces

9f9dd5a

Don't allow to use a missing namespace

f3edcb2

absoludity approved these changes Feb 3, 2020

View reviewed changes

Andres Martinez Gotor added 2 commits February 3, 2020 11:56

Review

95f07ed

Merge branch 'master' into warnMissingNS

2884907

andresmgot merged commit 4927c70 into master Feb 3, 2020

andresmgot deleted the warnMissingNS branch February 7, 2020 11:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return a valid error when using non-existing namespaces #1491

Return a valid error when using non-existing namespaces #1491

andresmgot commented Jan 30, 2020 •

edited

Loading

absoludity commented Jan 31, 2020

andresmgot commented Jan 31, 2020

andresmgot commented Jan 31, 2020

absoludity commented Feb 2, 2020

absoludity left a comment

absoludity Feb 2, 2020

absoludity Feb 3, 2020

andresmgot Feb 3, 2020

absoludity Feb 3, 2020

absoludity Feb 3, 2020

andresmgot Feb 3, 2020

andresmgot commented Feb 3, 2020

absoludity commented Feb 3, 2020

Return a valid error when using non-existing namespaces #1491

Return a valid error when using non-existing namespaces #1491

Conversation

andresmgot commented Jan 30, 2020 • edited Loading

Description of the change

Applicable issues

Additional information

absoludity commented Jan 31, 2020

andresmgot commented Jan 31, 2020

andresmgot commented Jan 31, 2020

absoludity commented Feb 2, 2020

absoludity left a comment

Choose a reason for hiding this comment

absoludity Feb 2, 2020

Choose a reason for hiding this comment

absoludity Feb 3, 2020

Choose a reason for hiding this comment

andresmgot Feb 3, 2020

Choose a reason for hiding this comment

absoludity Feb 3, 2020

Choose a reason for hiding this comment

absoludity Feb 3, 2020

Choose a reason for hiding this comment

andresmgot Feb 3, 2020

Choose a reason for hiding this comment

andresmgot commented Feb 3, 2020

absoludity commented Feb 3, 2020

andresmgot commented Jan 30, 2020 •

edited

Loading