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!: add a convenient way to add an openBIS cloud storage #3238

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

olloz26
Copy link
Collaborator

@olloz26 olloz26 commented Jul 11, 2024

BREAKING CHANGE: requires changes in the renku-data-services and renku-notebooks repositories

BREAKING CHANGE: requires changes in the renku-data-services and renku-notebooks repositories
@olloz26 olloz26 requested a review from a team as a code owner July 11, 2024 13:39
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Can't wait to try it with a working backend :)

I added a couple of comments inline.

@@ -119,6 +119,8 @@ export interface CloudStorageSchema {
hide?: boolean;
prefix: string; // ? weird naming; it's the machine readable name
position?: number;
convenientMode?: boolean; // ? Disables the advanced mode
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should include this; even if it makes little sense for users to mess with the config directly, they might still want to copy/paste the configuration string to pass it to other users to simplify setting up a working solution. I think we should remove it.

Comment on lines +1068 to +1074
const selectedSchema = useMemo(
() => getSchemaStorage(schema, !state.showAllSchema, storage.schema),
[schema, state.showAllSchema, storage.schema]
).find((s) => s.prefix === storage.schema);
if (selectedSchema && selectedSchema.readOnly) {
storage.readOnly = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is the idea to allow only read-only in every case? How do users change data there?

If that's the case, we should probably improve the UX because, with the current solution, we show a ticked read-only box that looks like an element users can interact with, but clicking doesn't do anything. I suggest:

  • Making the readOnly input... "readOnly" -- sorry for the pun, I mean that the element <input id="readonly [...] /> should have something like <input id="readonly [...] readOnly={selectedSchema.readOnly ?? false} />
  • Show a warning mentioning that there is no way to write with that external storage.

As I write these lines, I wonder why forcing the read-only flag here -- shouldn't that be down to the user's permissions? I assume some users can write.

@@ -62,6 +62,11 @@ export const CLOUD_STORAGE_OVERRIDE = {
"WebDAV compatible services, including PolyBox and SwitchDrive",
position: 2,
},
openbis: {
Copy link
Member

Choose a reason for hiding this comment

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

This warning is easy to address: you can add openbis to the array "skipWords" in the lining configuration in "client/.eslintrc.json"

@lorenzo-cavazzi lorenzo-cavazzi self-assigned this Aug 13, 2024
@lorenzo-cavazzi
Copy link
Member

@olloz26 Sorry for the delay; I'll review this soon.
Do we have a backend for this? Could you point me to the PRs on data-service and renku-notebooks?

P.S. It would be great if you could resolve the conflicts

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.

2 participants