-
Notifications
You must be signed in to change notification settings - Fork 24
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
Access Google Cloud Storage via NIO #6775
Conversation
@@ -311,7 +311,6 @@ function AddZarrLayer({ | |||
(userInput.indexOf("https://") !== 0 && userInput.indexOf("s3://") === 0) | |||
) { | |||
setSelectedProtocol(userInput.indexOf("https://") === 0 ? "https" : "s3"); | |||
setShowCredentialsFields(userInput.indexOf("s3://") === 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that all supported schemas now support their form of credentials so this is no longer needed
@frcroth I’d appreciate it if you could already have a look at the backend changes even though the frontend part is not yet complete. @philippotto agreed to adapt the front-end in the coming days. |
...sos-datastore/app/com/scalableminds/webknossos/datastore/storage/FileSystemCredentials.scala
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/s3fs/S3FileSystemProvider.java
Outdated
Show resolved
Hide resolved
Perfect 👍 Exploration works, but the data fetch requests don't really work for me. The front-end waits forever since the requests don't finish (until they'll probably time out). The console says:
Is there something wrong with my setup? I did a
Done 👍
Good point. I adapted the reset button to also reset the original input field. |
Thanks! The error reads to me as if blosc is not installed. That is a data (de)compression library. Does loading other blosc-compressed zarr datasets work for you? Could you try |
frontend/javascripts/admin/dataset/dataset_add_neuroglancer_view.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frontend code almost LGTM. I'll try to test and will report back.
const jsonString = await readFileAsText(file); | ||
return JSON.parse(jsonString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be guarded somehow? The page shouldn't crash if a file with the wrong format was uploaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this case. Currently, the page doesn't crash, but also nothing happens and no error is shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should give a proper error msg now :)
if (credentials) { | ||
return exploreRemoteDataset([datasourceUrl], { | ||
username: "", | ||
pass: JSON.stringify(credentials), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to find out which type credentials
has, but it's not strictly specified in wk. The only thing I found was Record<string, any>
in the NeuroglancerDatasetConfig
. Is there a more specific definition of the credential type? And is stringifying and passing all of it as the password here, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I found the link to the documentation which shows what the credentials file looks like (https://cloud.google.com/iam/docs/creating-managing-service-account-keys?hl=de#creating). In that case, there doesn't need to be a more specific type and it also makes sense to pass all of it as pass
here, so nevermind :)
Co-authored-by: Daniel <[email protected]>
Works very nicely 👍 Notes from testing:
Not from this PR, but I really like the error log, if the import doesn't work! 🥇 |
Done :) |
gs://
with optionalGoogleServiceAccountCredential
sURL of deployed dev instance (used for testing):
TODO
Steps to test:
gs://
zarr uri and ignoring the front-ends warningsIssues: