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

Lock dandisets during un-embargo #1957

Merged
merged 10 commits into from
Jun 21, 2024
Merged

Conversation

jjnesbitt
Copy link
Member

This change will help prevent race conditions that could arise if modifications are allowed while un-embargo is ongoing.

@jjnesbitt jjnesbitt requested a review from waxlamp June 14, 2024 18:45
Copy link
Member

@waxlamp waxlamp left a comment

Choose a reason for hiding this comment

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

This all looks good.

One note: I didn't realize that we prohibit publishing of embargoed Dandisets. I wonder if we should consider lifting that ban--I can see why it would be useful to be able to do so, though the wording we're using makes it confusing ("it's embargoed... how can I be publishing this?") when really "publish" mostly means to create an immutable snapshot.

@jjnesbitt
Copy link
Member Author

One note: I didn't realize that we prohibit publishing of embargoed Dandisets. I wonder if we should consider lifting that ban--I can see why it would be useful to be able to do so, though the wording we're using makes it confusing ("it's embargoed... how can I be publishing this?") when really "publish" mostly means to create an immutable snapshot.

I thought it was a hard requirement that published dandisets be open access, from more of a NIH policy standpoint.

@jjnesbitt jjnesbitt added patch Increment the patch version when merged release Create a release when this pr is merged labels Jun 14, 2024
@satra
Copy link
Member

satra commented Jun 14, 2024

I thought it was a hard requirement that published dandisets be open access, from more of a NIH policy standpoint.

yes, embargoed dandisets should not receive a published doi. publishing is only for non-embargoed dandisets.

in the future we may publish restricted dandisets, but we haven't implemented that yet.

to speak about roni's point of freezing versions on embargoed data, that's a different feature which we don't have. we should discuss whether that feature should even exist.

@jjnesbitt jjnesbitt requested a review from waxlamp June 17, 2024 16:04
@jjnesbitt
Copy link
Member Author

@waxlamp I had forgotten to include uploads as a part of the locking, as well as prohibit un-embargo in the first place if there were active uploads. Commits ef7c530 and b789645 cover that.

Copy link
Member

@waxlamp waxlamp left a comment

Choose a reason for hiding this comment

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

This looks good to me, with one requested change: within the exception names (and elsewhere as appropriate), please change the "UnEmbargo" capitalization to "Unembargo" (the word is spelled "unembargo", as "one word" and no hyphens, etc.).

@jjnesbitt jjnesbitt requested a review from waxlamp June 18, 2024 21:36
Copy link
Member

@waxlamp waxlamp left a comment

Choose a reason for hiding this comment

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

The UnEmbargo spelling within the exception classes should be shifted to Unembargo.

@jjnesbitt jjnesbitt force-pushed the lock-unembargoing-dandisets branch from 13965e9 to 492b48c Compare June 20, 2024 15:02
@jjnesbitt jjnesbitt requested a review from waxlamp June 20, 2024 15:02
@jjnesbitt jjnesbitt merged commit aed6c46 into master Jun 21, 2024
11 checks passed
@jjnesbitt jjnesbitt deleted the lock-unembargoing-dandisets branch June 21, 2024 00:45
@dandibot
Copy link
Member

🚀 PR was released in v0.3.87 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants