Skip to content
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

feat: save secrets on storage create #3274

Merged
merged 12 commits into from
Sep 5, 2024

Conversation

ciyer
Copy link
Contributor

@ciyer ciyer commented Aug 15, 2024

Allow storing credentials when a storage is created.

Screenshots

Add an option to save credentials to the last page of the add data source flow

image

When checked, the credentials will be stored after the data source is added. This can be verified by looking at the data source, the credential field will be displayed as

image

Testing

Add a data source that requires credentials to a project (or example an S3 or polybox data source). Check that the credentials used to test are stored. Also, check that there is no option to store credentials for a public data source (like a public S3 bucket).

/deploy renku-notebooks=master renku-data-services=main

@ciyer ciyer requested a review from a team as a code owner August 15, 2024 15:35
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 15, 2024 15:36 — with GitHub Actions Inactive
@RenkuBot
Copy link
Contributor

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

@ciyer ciyer marked this pull request as draft August 16, 2024 06:59
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from 00c822e to ed93aa9 Compare August 16, 2024 07:05
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 16, 2024 07:06 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from ed93aa9 to 852f6a1 Compare August 16, 2024 09:21
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 16, 2024 09:22 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 16, 2024 10:24 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from 74acb20 to 4547fd7 Compare August 19, 2024 08:06
@ciyer ciyer marked this pull request as ready for review August 20, 2024 09:42
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from 19da6bd to 2a1e82d Compare August 21, 2024 08:15
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 21, 2024 08:15 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 22, 2024 10:28 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 23, 2024 07:30 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 23, 2024 10:28 — with GitHub Actions Inactive
Copy link
Contributor

@andre-code andre-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good overall, but there are a few issues that need to be addressed:

  1. The secret title is currently displayed as the ID, but this ID is not shown in the data storage information. It would be better to use a more descriptive name, such as project/data-storageName.

Screenshot 2024-08-26 at 13 23 40

  1. When a secret is added while launching the store, it is saved as a user secret but doesn't show in the user secret list and does not indicate that it is saved in the storage information as <saved credential>.

  2. The icons are not properly aligned. I’ve left a comment in the code regarding this issue.

  3. The data storage sidebar should not open when clicking on the form label while editing a data storage.
    save credentials B

no show credentials saved

  1. Clicking on user secrets redirects to Renku 1.0. It would be better to have a Renku 2.0 route for user secrets when accessed from Renku 2.0.

  2. Here would be helpful to confirm that the secret is also created successfully.

Screenshot 2024-08-26 at 13 22 49

  1. The checkbox to store the secret is not shown when creating the data source, although the secret is still stored.

Please address these issues here or create new issues to handle them in a separate PR

@ciyer ciyer force-pushed the build/storage-credentials branch from efae265 to 30b1902 Compare August 27, 2024 06:41
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from 0ed4113 to 65dd221 Compare August 27, 2024 07:21
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 August 27, 2024 07:21 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from fef13b3 to 7b89dd8 Compare August 27, 2024 10:44
@ciyer ciyer force-pushed the build/storage-credentials branch from 30b1902 to 6fc5a82 Compare September 2, 2024 09:45
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from 69109b5 to a5e0b6f Compare September 2, 2024 09:46
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 September 2, 2024 09:46 — with GitHub Actions Inactive
@ciyer
Copy link
Contributor Author

ciyer commented Sep 2, 2024

Thanks for the review!

  1. When a secret is added while launching the store, it is saved as a user secret but doesn't show in the user secret list and does not indicate that it is saved in the storage information as <saved credential>.
  2. The icons are not properly aligned. I’ve left a comment in the code regarding this issue.
  3. [After skipping the credentials test, the] checkbox to store the secret is not shown when creating the data source, although the secret is still stored.

These bugs have been fixed.

  1. Here would be helpful to confirm that the secret is also created successfully.

I added this information to the dialog box

image

  1. The data storage sidebar should not open when clicking on the form label while editing a data storage.

You are right -- I was able to reproduce this bug, but it has been around for longer, and was not introduced by the changes here. I will address it in a separate PR.

  1. The secret title is currently displayed as the ID, but this ID is not shown in the data storage information. It would be better to use a more descriptive name, such as project/data-storageName.

  2. Clicking on user secrets redirects to Renku 1.0. It would be better to have a Renku 2.0 route for user secrets when accessed from Renku 2.0.

I agree, the current display of the secrets is not really usable. These limitations, however, were deemed acceptable in realizing the pitch, and improving the display will require changes to the backend because there is no efficient way to get the data source that a secret is for, so this will need to be handled separately, with support from the backend.

Please address these issues here or create new issues to handle them in a separate PR

@ciyer ciyer requested a review from andre-code September 2, 2024 10:12
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from a5e0b6f to dc7c454 Compare September 2, 2024 10:13
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 September 2, 2024 10:14 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the ciyer/storage-credentials-start branch from dc7c454 to 0b1d6a6 Compare September 2, 2024 10:28
@ciyer ciyer temporarily deployed to renku-ci-ui-3274 September 2, 2024 10:28 — with GitHub Actions Inactive
@andre-code
Copy link
Contributor

Thanks for applying those fixes!

I've noticed one issue and a couple of minor CSS adjustments that need attention:

[Bug] If the storage secret is removed from the secret page, the status on the project page doesn't update. As a result, when launching the session, the missing credential is not required.

no ask credentials

[CSS] In the storage credential modal, there's no vertical padding between fields. Please add some padding.

Screenshot 2024-09-02 at 13 39 11

[CSS] Similarly, on the data source page, there's no vertical padding between "Required credentials" and "Access mode." Please address this as well.

Screenshot 2024-09-02 at 13 39 22

@ciyer ciyer temporarily deployed to renku-ci-ui-3274 September 4, 2024 09:07 — with GitHub Actions Inactive
@ciyer
Copy link
Contributor Author

ciyer commented Sep 4, 2024

Thanks for the review, I have fixed the issues you have raised.

[Bug] If the storage secret is removed from the secret page, the status on the project page doesn't update. As a result, when launching the session, the missing credential is not required.

Fixed in 311382d

[CSS] In the storage credential modal, there's no vertical padding between fields. Please add some padding.

image

Fixed in 65ce6e4

[CSS] Similarly, on the data source page, there's no vertical padding between "Required credentials" and "Access mode." Please address this as well.

image

Fixed in 212c0c5

@ciyer ciyer temporarily deployed to renku-ci-ui-3274 September 5, 2024 13:01 — with GitHub Actions Inactive
Copy link
Contributor

@andre-code andre-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🚀

@ciyer ciyer merged commit 5f51710 into build/storage-credentials Sep 5, 2024
10 of 18 checks passed
@ciyer ciyer deleted the ciyer/storage-credentials-start branch September 5, 2024 15:26
@RenkuBot
Copy link
Contributor

RenkuBot commented Sep 5, 2024

Tearing down the temporary RenkuLab deplyoment for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants