SwissDataScienceCenter / renku-ui

The web frontend of the Renku platform
https://renkulab.io
Apache License 2.0
13 stars 6 forks source link

Allow remembering storage credentials as secrets #3224

Closed ciyer closed 24 minutes ago

ciyer commented 1 week ago

Changes

This PR supports

Testing

After creating a project that includes Cloud Storage which requires credentials (e.g., a private S3 bucket) and a Session, when you start the session on this project, you will be prompted to provide your credentials with an option to save them.

image

Once the session has been launched, it is possible to go to the Secrets (V1) page and see the secrets for the storage.

image

Some scenarios to test include:

/deploy renku-data-services=pitch/save-storage-credentials-api

RenkuBot commented 1 week ago

You can access the deployment of this PR at https://renku-ci-ui-3224.dev.renku.ch

ciyer commented 4 days ago

Great to see the code in action! It's mostly looking good, but a few issues need addressing:

  • Adding a new general secret key doesn't work; it errors out asking for the kind of secret.

Good catch! I have fixed this in 940a4c77d656755ae71f8cb4801232a7e9142c67.

  • When launching a session with more than 1 data sources, it only send in the cloudstorage array the last data source in the list.

Can you elaborate on this? I just tested a project with multiple data sources and it worked as expected.

  • UX Issue: If a data source has multiple secret keys, and some are already stored, it still asks for all keys again. It should only request the missing keys. What do you think?

I agree that the UX in this case could be improved, but I will fix it in #3229, where I am modifying the dialog to allow display of stored secrets.

  • UX Issue: When launching a session with multiple data sources, the "continue" or "skip" functionality isn't clear. Will it skip the problematic data source or try to use it anyway? Adding a clarification to the message like "The data source could not be mounted. Please retry with different credentials, or skip." might help.

I do not follow. This is what I see right now:

image

Do you think a different message should be shown?

✨ It'd be great if the data source view showed which credentials already have a secret value stored and linked to the secret page if accessible. This helps users know where to find and manage them. Makes sense? I see there's already an endpoint to check that 🙂

Agreed! That is what is being done in #3229.

andre-code commented 1 day ago

Thanks for fixing the issues.

Regarding the cases where not all data sources are being sent, I'm also confused. In this example, the launcher asked for keys of the datasources giab and giab2. It couldn't connect in both cases, but it sent only one of the data sources. Please check the payload; it should send none since it couldn't connect with any of the data sources, and the user selected the skip option in both cases. Or should it send both even if it didn't connect?

Additionally, I think we should extend the message to explain what "skip" means: does it skip the connection attempt and continue trying to mount the data source, or does it not attempt mount the data source at all?

launch with storage 2

I hope it clarifies a bit better ✨

ciyer commented 1 day ago

Thanks for fixing the issues.

Regarding the cases where not all data sources are being sent, I'm also confused. In this example, the launcher asked for keys of the datasources giab and giab2. It couldn't connect in both cases, but it sent only one of the data sources. Please check the payload; it should send none since it couldn't connect with any of the data sources, and the user selected the skip option in both cases. Or should it send both even if it didn't connect?

Additionally, I think we should extend the message to explain what "skip" means: does it skip the connection attempt and continue trying to mount the data source, or does it not attempt mount the data source at all?

I hope it clarifies a bit better ✨

Yes, thanks for the clarification! This has prompted me to think a bit more about how this should work. I have decided that the skip button should skip the test, but still try to mount the cloud storage at startup time. The reason for this is that the logic for the test is not exactly the same as what happens when the session starts, and there are situations where the test fails, but the storage would still mount.

Given that, I have adapted the user feedback.

  1. Check the connection, but get a failure image
image
  1. Skip the test without checking the connection
image

Given that, all the data sources should be sent to the backend, which should try to mount them at session start.

RenkuBot commented 23 minutes ago

Tearing down the temporary RenkuLab deplyoment for this PR.