-
Notifications
You must be signed in to change notification settings - Fork 532
IQSS/11909-Update netcdf library #11910
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/11909-Update netcdf library #11910
Conversation
pdurbin
left a comment
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 didn't test netcdf (we should, obviously) at all but I'm going to approve this. It has some nice cleanup: removing com.beust.jcommander.Strings.
I played around with but didn't commit the following test I added to PidUtilTest locally and I can see the String change working properly:
@Test
public void testGetProviders() throws IOException {
JsonObject job = PidUtil.getProviders();
System.out.println("providers " + JsonUtil.prettyPrint(job));
}
| providerSpecification.add("excludedSet", String.join(",", excludedSet)); | ||
| return providerSpecification.build(); | ||
| } | ||
|
|
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.
Jenkins is failing but I think it's unrelated to this PR. I just kicked off another run: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-11910/3/
The old copy is apparently back or at least we're able to build fine now without this PR. See #11919, for example. As discussed at standup we closed the following "can't build" issue now that we can build: |
What this PR does / why we need it: This PR updates the library needed for the netcdf ingest functionality. The old copy of the library has become unavailable causing build errors. We have also seen warnings about bouncycastle being unavailable (it is now bouncy) - not clear if this update will fix that.
Which issue(s) this PR closes:
Special notes for your reviewer: The change to the AbstractPidProvider is switching from a Strings class that was evidently coming from one of the libraries previously used in the
Suggestions on how to test this: If we can now build, the PR serves its purpose. Testing the netcdf ingest process (regression testing) should also be done.
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?:
Additional documentation: