Refactor default namespace resolution #2161
Conversation
dashboard/src/shared/Namespace.ts
Outdated
| @@ -50,3 +51,18 @@ export default class Namespace { | |||
| export const definedNamespaces = { | |||
| all: "_all", | |||
| }; | |||
|
|
|||
| export function getCurrentNamespace(currenNS: string, availableNS: string[]) { | |||
There was a problem hiding this comment.
Minor typo, perhaps you mean currentNS here?
|
I am debugging an issue in the e2e test since it seems that it's clicking in the Catalog link before the default namespace is resolved, causing a 404 :) [EDIT] I've made sure we are not rendering the links if the namespace info has not been populated. I had to move the namespace fetching from |
| expect(items).not.toExist(); | ||
| }); | ||
|
|
||
| it("should skip the links the namespace info is not available", () => { |
There was a problem hiding this comment.
links if the namespace info...
| authenticated={true} | ||
| cluster={""} | ||
| currentNamespace={""} | ||
| authenticated={false} |
There was a problem hiding this comment.
I'm confused what this test is now testing compared to the following? It looks like they are both testing that the redirect is rendered when unauthed (and that the value of the state should be irrelevant - so only one test required?)
There was a problem hiding this comment.
yes, the only difference is that the cluster and currentNamespace info is populated, which should be ignored. Updating the test description for being more clear.
| exact={true} | ||
| path={reposPath} | ||
| component={RepoListContainer} | ||
| /> |
There was a problem hiding this comment.
Was this a temporary route we'd added during the switch to clustered/namespaced routes? Right, then we've updated reposPath but never removed the now redundant route?
There was a problem hiding this comment.
seems so, there was no reason to keep it as special case.
| @@ -50,3 +51,18 @@ export default class Namespace { | |||
| export const definedNamespaces = { | |||
| all: "_all", | |||
| }; | |||
|
|
|||
| export function getCurrentNamespace(currentNS: string, availableNS: string[]) { | |||
| if (currentNS) { | |||
There was a problem hiding this comment.
Don't understand why we even have currentNS as an arg - why call this function if we already have the currentNS? EDIT: I see - the call-site doesn't need to check, but can just pass in the state. OK.
There was a problem hiding this comment.
yes, it was to avoid doing the check every time I had to call this function.
| const tokenNS = Auth.defaultNamespaceFromToken(Auth.getAuthToken() || ""); | ||
| if (tokenNS) { | ||
| // Return the default namespace in the token | ||
| return tokenNS; |
There was a problem hiding this comment.
We should validate that this ns is in availableNS - we had a recent report because in Edukates the token is from a different namespace to which the user has no access.
Description of the change
This is the first PR, preparing a following-up to be able to store the last used namespace in the browser storage. In this PR:
authaction will set/modify thecluster(or namespace) state.cluster.currentNamespaceso components don't need to look at other places for this info.Benefits
Getting rid of some spaghetti code :)
Possible drawbacks
The application will now wait for the API to return the existing namespaces before selecting one (which includes a minimal delay when loading the app).
Applicable issues