-
Notifications
You must be signed in to change notification settings - Fork 0
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
Make deleted Dandisets private #57
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
- Coverage 74.62% 74.36% -0.27%
==========================================
Files 17 17
Lines 2806 2816 +10
Branches 624 629 +5
==========================================
Hits 2094 2094
- Misses 533 542 +9
- Partials 179 180 +1 ☔ View full report in Codecov by Sentry. |
@@ -90,25 +90,47 @@ async def update_from_backup( | |||
if d.embargo_status is EmbargoStatus.OPEN | |||
else "private" | |||
) | |||
if not dandiset_ids and self.config.gh_org is not None: |
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.
So we will delete only when ran on all, not specific dandisets . Should be fine for the majority of the use cases but I am afraid it might cause confusion since I will forget such peculiarity and might need/try to run on specific one(s). Is it too convoluted to make it also work if dandiset_ids were provided?
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.
Is it too convoluted to make it also work if dandiset_ids were provided?
In that case, what deletions would you expect to be detected? Do you want all recently-deleted Dandisets to be marked private or only those that are included in the given list of IDs? For the latter option, note that requesting a backup of a nonexistent or deleted Dandiset currently results in an error, and I currently don't see a decent way to turn that error into some "Dandiset was deleted" marker.
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 was thinking about "latter".
interesting. Ok -- so if we error upon explicit specification of a deleted dataset, then I guess it is ok as now. Thank you!
is it possible to get a quick test added to ensure we cover this logic? |
Not really. This logic only runs if |
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.
Let's proceed then without test. Merge on your convenience and ideally supervise the initial run which might need to perform those changes for some already existing dandisets
@yarikoptic Run completed. |
Closes #55.