-
Notifications
You must be signed in to change notification settings - Fork 494
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
IQSS/9095-dvwebloader integration #9096
IQSS/9095-dvwebloader integration #9096
Conversation
IQSS/9126-allow_workflow_tokens_in_access_api
added to sprint Dec 15, 2022 |
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'm leaving some comments in this review.
Before testing, I merged the latest from develop into this PR. The files never showed up in the dataset. I'll try to circle back and troubleshoot some more.
I pushed the merge as well as a small doc update because without asking I wasn't able to figure out how to get the "upload a folder" UI to appear. Before the merge, all API tests we passing, which is good.
Here are screenshots from my testing:
Still no files after a refresh
/** | ||
* The URL for the DvWebLoader tool (see github.com/gdcc/dvwebloader for details) | ||
*/ | ||
WebloaderUrl |
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.
As discussed yesterday, we should probably get in the habit of using MPCONFIG, and not just to make @poikilotherm happy. 😄
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.
If I read the existing code right, the new classes can replace a JVM property but code to replace settings (and keep the ability to dynamically change them) isn't in place. If it is, please point me at an example. (FWIW: #9175 was my first mpconfig jvm option and it appears to work so I'll try to keep adding mpconfig for those).
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 see. @poikilotherm I just moved this to QA but if you have ideas, please let us know. Thanks.
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
I added this to a list of issues here: |
@qqmyers explained that the reason files weren't appearing in my dataset is that I didn't have CORS enabled on my S3 bucket. The docs on this are in pretty bad shape (documented in the dev guide rather than the installation guide) but I tried to improve them a bit in 78bc5a5. Here's the working sequence (showing the console to make sure the uploads to S3 are happening): Upload completeNo files on dataset page until you refreshAfter clicking refresh, the files are presentI think this is ready for QA. Great feature. |
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.
As I said in my last comment, I feel this is ready for QA.
@poikilotherm sorry, @qqmyers and I are not sure how to apply MPCONFIG to this one. Please jump in that review comment thread if you have ideas. Thanks.
What this PR does / why we need it: This PR allows an admin to enable and configure a connection with the dvwebloader tool to allow upload of a whole folder tree of files to Dataverse.
Which issue(s) this PR closes:
Closes #9095
Special notes for your reviewer: As with Globus, dvwebloader started as a dataset-level external tool and the limitations there (doesn't appear until one file has been uploaded, appears regardless of whether the dataset uses a store that supports it, isn't in an obvious place for uploads) apply for dvwebloader as well.
FWIW: The dvwebloader is relatively simple at present but will be gaining functionality over time (e.g. implementing more things that the command-line DVUploader does.
Suggestions on how to test this: Follow the docs to configure the dvwebloader and create a dataset using an S3/direct upload store. Verify that upload through the tool works. Regression - verify normal upload is not affected.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: probably should announce it.
Additional documentation: