Skip to content

Comments

Make MDC DataCite URl configurable#6265

Merged
kcondon merged 9 commits intoIQSS:developfrom
QualitativeDataRepository:IQSS/6137
Oct 22, 2019
Merged

Make MDC DataCite URl configurable#6265
kcondon merged 9 commits intoIQSS:developfrom
QualitativeDataRepository:IQSS/6137

Conversation

@qqmyers
Copy link
Member

@qqmyers qqmyers commented Oct 9, 2019

New Contributors

Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Note that we use the "closes" syntax below to trigger Github's automation to close the corresponding issue once the pull request is merged.

Thanks for your contribution to Dataverse!

Related Issues

Pull Request Checklist

@dataversebot
Copy link

Can one of the admins verify this patch?

@coveralls
Copy link

coveralls commented Oct 9, 2019

Coverage Status

Coverage decreased (-0.01%) to 19.462% when pulling 4bf2ad2 on QualitativeDataRepository:IQSS/6137 into 3bf9091 on IQSS:develop.

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 so I'm passing to QA but there's a fair amount to test here (not that that's bad, just sayin'):

  • The installer has been updated.
  • A new JVM option has been added.
  • docker-aio has been updated (used for testing the JVM option, I assume)
  • The docs have been updated.

I didn't build or test any of the above but all the changes make sense to me.

I guess my only feedback for @qqmyers is that it might be nice to update the Make Data Count documentation, maybe in the Admin Guide or the Dev Guide to mention the new JVM option. Both guides have pages on Make Data Count.

@pdurbin pdurbin removed their assignment Oct 10, 2019
@kcondon
Copy link
Contributor

kcondon commented Oct 10, 2019

@qqmyers @pdurbin

./install
String found where operator expected at ./install line 146, near "'DOI_MDCBASEURL'"
(Missing semicolon on previous line?)
String found where operator expected at ./install line 175, near "'DOI_MDCBASEURL'"
(Missing semicolon on previous line?)
syntax error at ./install line 146, near "'DOI_MDCBASEURL'"
syntax error at ./install line 175, near "'DOI_MDCBASEURL'"
Execution of ./install aborted due to compilation errors.

@pdurbin
Copy link
Member

pdurbin commented Oct 11, 2019

run tests

@kcondon
Copy link
Contributor

kcondon commented Oct 14, 2019

I have built and checked the docs, tested the endpoints. Had trouble running the installation script -think it may just be missing a , when adding an addition element to list. Have not checked whether Dataverse actually uses the new MDC setting.

@pdurbin
Copy link
Member

pdurbin commented Oct 15, 2019

Yeah, it looks like there may be two places in the Perl script where commas are missing.

@qqmyers qqmyers removed their assignment Oct 21, 2019
@djbrooke
Copy link
Contributor

Thanks @qqmyers. Ready for us to take another look?

@qqmyers
Copy link
Member Author

qqmyers commented Oct 22, 2019

@djbrooke - Yep. BTW - When the fix is pretty obvious as it was here (two commas), you guys should feel free to make the change and keep going. (You're welcome to edit any time but here it definitely seems like more effort to document the issue, reassign, wait, etc.) I'm happy to support PRs when that makes less work for you guys, but no need for more work either.

@djbrooke
Copy link
Contributor

Thanks @qqmyers, I'll try to do a better job keeping on top of when small changes need to be done and having us do them.

@pdurbin pdurbin self-assigned this Oct 22, 2019
@pdurbin
Copy link
Member

pdurbin commented Oct 22, 2019

@qqmyers I'm going to work on one such small change. @kcondon requested some logging.

Also, a little bird told me that there's a bug in the changes to the Perl script. 😄

Thanks, as always, for hacking on Dataverse! 🎉

@kcondon
Copy link
Contributor

kcondon commented Oct 22, 2019

@qqmyers @pdurbin

installer:
-JVM option name transposed: mdcbaseurl is set as basemdcurl instead.
-Default installation value for mdcbaseurl is not set (empty), need to also set the gfish env value.

Jim, are you able to test run the installer on your end?

Phil, thanks for adding the logger so I can check which url is used.

@qqmyers
Copy link
Member Author

qqmyers commented Oct 22, 2019

@kcondon - just updated the naming for the jvm option. (the api code was also different). I'm not really able to run the installer - really working on our branch and using the auto build/deploy process we have so I'm really guessing on changes to the various scripts.
W.r.t. a default value - I made it backwards compatible, so if you don't set it, the hardcoded value from earlier versions are used. If that's removed, the update instructions will need to include the step to set it.

@qqmyers
Copy link
Member Author

qqmyers commented Oct 22, 2019

@pdurbin - thanks for adding logging, and, as for hacking on Dataverse, at least you get a T-shirt this month :-)

@landreev
Copy link
Contributor

For the main installer script to work, could you please add the following 2 lines to scripts/installer/install, at line 1104:

    $ENV{'DOI_BASEURL'} = $CONFIG_DEFAULTS{'DOI_BASEURL'};
    $ENV{'DOI_MDCBASEURL'} = $CONFIG_DEFAULTS{'DOI_MDCBASEURL'};

(it was missing it for the DOI_BASEURL variable too, yes;)

I would do it myself, but the branch is in your repository.
Thanks.

@pdurbin
Copy link
Member

pdurbin commented Oct 22, 2019

I would do it myself, but the branch is in your repository.

Yeah, I got "ERROR: Permission to QualitativeDataRepository/dataverse.git denied to pdurbin." when I tried to git push QualitativeDataRepository IQSS/6137 (which is fine) so I just created this pull request: QualitativeDataRepository#26

I haven't looked at the Perl script yet and don't plan to unless someone wants me to.

@pdurbin pdurbin closed this Oct 22, 2019
@pdurbin pdurbin reopened this Oct 22, 2019
@pdurbin
Copy link
Member

pdurbin commented Oct 22, 2019

Sorry, it is WAY to easy to accidentally close a pull request. Re-opened! I mis-clicked. 😞

@pdurbin
Copy link
Member

pdurbin commented Oct 22, 2019

@kcondon in order to bump up the logging to see "Requesting citations from https://api.datacite.org/events?doi=10.5072/FK2/BL2IBM&source=crossref" (or whatever) in server.log, you can run this:

asadmin set-log-levels edu.harvard.iq.dataverse.api.MakeDataCountApi=FINE

@qqmyers
Copy link
Member Author

qqmyers commented Oct 22, 2019

Hmm - it's supposed to work. When I create the PRs I check the box to allow editing as in
image
This image is a test where I was tried our v4.17-qdr branch and you can see it says people with write on IQSS/dataverse (not QDR/dataverse) can commit to the branch in the PR. Could it be only by editing in the github website?

@djbrooke
Copy link
Contributor

@qqmyers FWIW I tried update two weeks ago what I eventually mentioned in #6265 (comment) and was also denied. I assumed it was due to me being not good at Github. :) Sorry, I should have raised the flag then.

@pdurbin pdurbin removed their assignment Oct 22, 2019
@kcondon kcondon merged commit 690e081 into IQSS:develop Oct 22, 2019
@kcondon kcondon self-assigned this Oct 22, 2019
@djbrooke djbrooke added this to the 4.18 milestone Oct 23, 2019
@qqmyers qqmyers deleted the IQSS/6137 branch May 17, 2024 18:40
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.

MakeDataCountApi is hardcoded to production datacite server

7 participants