Skip to content

4949 download package s3#5359

Merged
kcondon merged 36 commits intodevelopfrom
4949-download-package-s3
Dec 14, 2018
Merged

4949 download package s3#5359
kcondon merged 36 commits intodevelopfrom
4949-download-package-s3

Conversation

@matthew-a-dunlap
Copy link
Contributor

Note: There are still two changes incoming for this PR.

  1. Documentation around the :DownloadMethod changes and a .rst page on how to set up dcm s3.
  2. A bump to the dcm version number once this pr is merged S3 package zip sbgrid/data-capture-module#35

Related Issues

Pull Request Checklist

matthew-a-dunlap and others added 25 commits November 1, 2018 16:24
This move allows that code to be reused in the new package popup

This also contains a bit of render logic around s3 package UI
There was a bug in the previous story where the imported dcm s3 files did not have an S3 identifier.
Wired up for all flows, and multiple update bugs fixed.
Will need to be bumped again after PR
Added the section "Downloading a Dataverse Package via URL". This should cover this feature for the User Guide.
Based on Phil's review, changed the docs to no longer wildly overestimate Wget's capabilities ;)
in the future this code may be used for non-s3 packages and so should be more agnostic.
@coveralls
Copy link

coveralls commented Nov 30, 2018

Coverage Status

Coverage decreased (-0.03%) to 17.667% when pulling 5537175 on 4949-download-package-s3 into 15397f8 on develop.

xmlns:o="http://omnifaces.org/ui"
xmlns:iqbs="http://xmlns.jcp.org/jsf/composite/iqbs">
<p:outputPanel>
<div id="fileInfoInclude-filesTable" class="col-sm-12 row">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to suggest a simple name change to this xhtml file, so that it will be grouped with the other file related files in the webapp directory. Something like "file-info-fragment" would do it.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I see that "a .rst page on how to set up dcm s3" is coming, which is awesome and I'll wait to approve this pull request until then.

Editing still needed
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Overall, looks just fine but I noted a few changes we could make.

}
}

logger.log(Level.INFO, "Checksum value for the package in Dataset {0} is: {1}",
Copy link
Member

Choose a reason for hiding this comment

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

Should this be FINE instead of INFO since it's the happy path?

*/
RSYNC("rsal/rsync"),
NATIVE("NATIVE");
NATIVE("native/http");
Copy link
Member

Choose a reason for hiding this comment

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

Should we also update the tests in DataCaptureModuleUtilTest.java?

}

public boolean isHTTPDownload()
{
Copy link
Member

Choose a reason for hiding this comment

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

We could move this curly to the line above, if we care to.

<param-name>javax.faces.PROJECT_STAGE</param-name>
<!-- <param-value>Development</param-value>-->
<param-value>Production</param-value>
</context-param>
Copy link
Member

Choose a reason for hiding this comment

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

I'm very happy to see this in here because I always forget how to enable debugging in JSF.

xmlns:iqbs="http://xmlns.jcp.org/jsf/composite/iqbs">
<p:outputPanel>
<div id="fileInfoInclude-filesTable" class="col-sm-12 row">
<div class="pull-left col-file-thumb">
Copy link
Member

Choose a reason for hiding this comment

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

I just wanted to note that @mheppler made a comment above about file name conventions for xhtml files. From my perspective, we are not at all consistent. My approach would probably be to start by asserting a preference in the dev guide on the "Coding Style" page. Then we could work toward renaming existing files to comply with the naming standard.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Looks good as of c891407

@kcondon kcondon merged commit 88c2494 into develop Dec 14, 2018
@kcondon kcondon deleted the 4949-download-package-s3 branch December 14, 2018 17:11
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.

6 participants