Skip to content
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

ci: fix changed resolved ref for dataverse test, fix recent github ratelimit PR #1615

Merged
merged 5 commits into from
Jan 7, 2023

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Jan 4, 2023

There was a new version of a DOI that was tested against, so "resolved_ref" transitioned from v3.0 to v4.0 it seems. The last update to this DOI was several years ago. I've made it into a regexp now though so if new releases happen, it should not break this again.

@@ -171,7 +171,7 @@ async def test_hydroshare_doi():
[
"10.7910/DVN/TJCLKP",
"10.7910/DVN/TJCLKP",
"3035124.v3.0",
"3035124.v4.0",
Copy link
Member

Choose a reason for hiding this comment

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

@Xarthisius @pdurbin You were involved in the PR that added support for Dataverse and this test in
#969

Is this change in the resolved reference expected? Is there a way to make this constant so it doesn't change again in future?

Copy link
Contributor

Choose a reason for hiding this comment

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

@manics hi! Sort of? 😄 I mean, I don't know anything about the test (@Xarthisius might) but I did publish a new version (4) of that 10.7910/DVN/TJCLKP dataset yesterday.

3035124 is the database id of the dataset so that shouldn't change.

I'm not sure if this helps but Dataverse supports the idea of getting the latest published version of a dataset with :latest-published: https://guides.dataverse.org/en/5.12.1/api/native-api.html#datasets ... perhaps this could be a way of avoiding hard coding a value. Please let me know if you have any questions about the Dataverse API or if I can help at all. Thanks for looking into this!

Copy link
Member Author

Choose a reason for hiding this comment

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

Is "10.7910/DVN/TJCLKP" associated with v3 or the latest version? Now we get the latest version from that as a reference given to binderhub, when we may or may not have expected we would get v3 even if there was a newer version.

Copy link
Contributor

Choose a reason for hiding this comment

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

@consideRatio I'm not sure I'd put it quite that way.

"10.7910/DVN/TJCLKP" is associated with all versions of the dataset, including the latest version and previous versions (v3, v2, etc). Dataverse doesn't mint DOIs for each version of a dataset (though this functionality has certainly been requested for quite a while).

I still think :latest-published (above) might help but there's also a Dataverse API to list each version of a dataset, if that's of interest: https://guides.dataverse.org/en/5.12.1/api/native-api.html#list-versions-of-a-dataset

Sorry if I'm not quite following. If you're getting the latest version from "10.7910/DVN/TJCLKP" I'm not sure why v3 is hard coded (and updated to v4 in this PR).

Copy link
Member

Choose a reason for hiding this comment

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

IQSS/dataverse#4499 explains it, I assumed Dataverse supported exact DOIs resolving to a particular version for reproducibility 😞 .

I can think of two options here (I'm happy with whatever you prefer):

  • Change this to v4 as in the PR
  • Change this (and the test case below) to a regex match for v\d+

Either way, adding a comment to this test case explaining all this would be helpful for future readers!

Copy link
Contributor

Choose a reason for hiding this comment

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

@manics if I get a vote, I much prefer the regex option. Publishing a new version of my dataset shouldn't break anything. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Regex added and I've included a comment based on your comment here @pdurbin - thank you!!

@consideRatio consideRatio marked this pull request as draft January 5, 2023 21:13
@consideRatio consideRatio changed the title ci: updated resolved ref for dataverse test ci: fix changed resolved ref for dataverse test, fix recent github ratelimit PR Jan 5, 2023
@consideRatio
Copy link
Member Author

@manics I squeezed in a refactor of a function that could lead to a silent error and a complementary fix for #1612.

This is done now I think!

@consideRatio consideRatio marked this pull request as ready for review January 5, 2023 21:27
Copy link
Contributor

@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.

Look great! So much better than hard coding a version. Ship it! :shipit: 🚀

binderhub/tests/test_repoproviders.py Outdated Show resolved Hide resolved
binderhub/tests/test_repoproviders.py Outdated Show resolved Hide resolved
binderhub/tests/test_repoproviders.py Outdated Show resolved Hide resolved
Co-authored-by: Simon Li <orpheus+devel@gmail.com>
@manics manics merged commit 7a558f0 into jupyterhub:main Jan 7, 2023
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jan 7, 2023
jupyterhub/binderhub#1615 Merge pull request #1615 from consideRatio/pr/fix-dataverse
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dataverse test failure in main branch
3 participants