Arvados

Also weird things happen if configuration changes inconsistently with what's in local storage.

workbench2 repository

15530-wb2-logincluster @ commit:d866a8ad7d9d8f48c761fa7ea8ea96b17cbfdb2f

To test this you need a federation with LoginCluster set, eg the "fedbox" cluster set up to test arv-federation-migrate.

• wb2 "homecluster" is set to value of logincluster
• don't display dropdown to select alternate home cluster for login and linking when logincluster is non-empty
• when loading "sessions" from local storage, clear "active" and "validated" flags and re-initialize
• when searching and logincluster is set, send unsalted token
• when logging into the head logincluster, send unsalted token to other workbenches

I was going to address #15709 on this branch. I looked at it, remoteHosts already adds the local cluster entry (whether it shows up in the cluster RemoteHosts config or not). I did notice that remoteHostConfigs did not include the local config, and so I added that.

I think this is ready to be looked at.

Peter Amstutz wrote:

I was going to address #15709 on this branch. I looked at it, remoteHosts already adds the local cluster entry (whether it shows up in the cluster RemoteHosts config or not). I did notice that remoteHostConfigs did not include the local config, and so I added that.

I think this is ready to be looked at.

• Looks like you have to update some of the tests.

• Logging into a cluster that isn't the LoginCluster presents the user with a button that says "Login to X with user from Y". This may be confusing for users in a federation with LoginCluster set. I think its worth conditionally changing that text to just say "Login to X" if there is a LoginCluster.

• The Site-Manager link in search results needs some explanation. Maybe something like: "To manage clusters that are searched see the Site Manager", where instead of a Link, its a Button styled like the others. A right align might work to separate it from the rest of the text on that line also.

• Unfortunately, I think this breaks account linking. When trying to link an account on a non-LoginCluster, it thinks every user is a remote user (uuid points to LoginCluster) and expects you to link to a local account, which wont exist if users are migrated. You probably just have to change link-account-panel-actions.tsx line 205 to compare the uuid against the LoginCuster if it exists.

• Nit: site-manager-panel-root.tsx ln 164: </Typography> needs a tab.

Eric Biagiotti wrote:

Peter Amstutz wrote:

I was going to address #15709 on this branch. I looked at it, remoteHosts already adds the local cluster entry (whether it shows up in the cluster RemoteHosts config or not). I did notice that remoteHostConfigs did not include the local config, and so I added that.

I think this is ready to be looked at.

• Looks like you have to update some of the tests.

Fixed.

• Logging into a cluster that isn't the LoginCluster presents the user with a button that says "Login to X with user from Y". This may be confusing for users in a federation with LoginCluster set. I think its worth conditionally changing that text to just say "Login to X" if there is a LoginCluster.

Now it just says "Log in" when there are not any options. The cluster id is still visible in the upper left.

• The Site-Manager link in search results needs some explanation. Maybe something like: "To manage clusters that are searched see the Site Manager", where instead of a Link, its a Button styled like the others. A right align might work to separate it from the rest of the text on that line also.

Tweaked it a little bit. It's hard to make it a button and also explain why it is there, so it is just a link.

• Unfortunately, I think this breaks account linking. When trying to link an account on a non-LoginCluster, it thinks every user is a remote user (uuid points to LoginCluster) and expects you to link to a local account, which wont exist if users are migrated. You probably just have to change link-account-panel-actions.tsx line 205 to compare the uuid against the LoginCuster if it exists.

Now disables account linking entirely unless you're on the home cluster.

• Nit: site-manager-panel-root.tsx ln 164: </Typography> needs a tab.

Fixed.

15530-wb2-logincluster @ commit:df4133dde10614e53a41b16a5c6062c3d1777059

• On a remote clusters link account page, users don't know what the login cluster is, so its not worth mentioning especially since we give them a link anyway. I would change the text to "Account linking is unavailable on this cluster. You must visit zzzzz to link accounts." Or something like that.

• On the search results page, if there is only 1 cluster, I would take out the whole heading saying what cluster is being searched and the link to site manager. At the very least, make "clusters" singular.

• Whitespace issues in link-account-panel-root.tsx and site-manager-panel-root.tsx.

Eric Biagiotti wrote:

• On a remote clusters link account page, users don't know what the login cluster is, so its not worth mentioning especially since we give them a link anyway. I would change the text to "Account linking is unavailable on this cluster. You must visit zzzzz to link accounts." Or something like that.

Now it says "Please visit cluster xxxxx to perform account linking"

• On the search results page, if there is only 1 cluster, I would take out the whole heading saying what cluster is being searched and the link to site manager. At the very least, make "clusters" singular.

I reworked this (and ended up tinkering with the rendering some more).

• Whitespace issues in link-account-panel-root.tsx and site-manager-panel-root.tsx.

Fixed? I think? Relying on auto-indent from the language server.

Peter Amstutz wrote:

Eric Biagiotti wrote:

• On a remote clusters link account page, users don't know what the login cluster is, so its not worth mentioning especially since we give them a link anyway. I would change the text to "Account linking is unavailable on this cluster. You must visit zzzzz to link accounts." Or something like that.

Now it says "Please visit cluster xxxxx to perform account linking"

• On the search results page, if there is only 1 cluster, I would take out the whole heading saying what cluster is being searched and the link to site manager. At the very least, make "clusters" singular.

I reworked this (and ended up tinkering with the rendering some more).

• Whitespace issues in link-account-panel-root.tsx and site-manager-panel-root.tsx.

Fixed? I think? Relying on auto-indent from the language server.

This LGTM. I won't hold up the merge if you think link-account-panel-root.tsx is OK the way it is (I don't). Going forward, if the language server is mangling code, please don't commit it. It obscured your actual changes and made review more difficult.

Eric Biagiotti wrote:

This LGTM. I won't hold up the merge if you think link-account-panel-root.tsx is OK the way it is (I don't).

This comment is vague. Are you referring to the code formatting or something functional?

Going forward, if the language server is mangling code, please don't commit it. It obscured your actual changes and made review more difficult.

Well the review comment was non-specific "whitespace issues". So maybe I should have just left it alone rather than bulk reformat it. Unfortunately the language server I'm using has different ideas about how things should be indented than the original developers, perhaps we need to harmonize on a code formatter or particular settings.