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

Add force argument in ingest_zarr_archive #1010

Merged
merged 2 commits into from
Apr 12, 2022
Merged

Conversation

jjnesbitt
Copy link
Member

Addresses underlying cause of #1004.

@dchiquito Right now, the management command defaults to force=False. I'd think most of the time we're using the management command, we'd want to use force=True. Do you think we should default force=True in the management command? I'm leaning towards yes.

I'm also wondering how we can improve the process of checksum updates. That is, if there is ever a need again to update the checksum format, it'd be good to have some automated process around this, rather than someone needing to manually run a management command. I get the feeling that any automated solution would be overly costly in the average case of not needing to update. @dchiquito any thoughts?

@jjnesbitt jjnesbitt requested a review from dchiquito April 6, 2022 21:02
Copy link
Contributor

@dchiquito dchiquito left a comment

Choose a reason for hiding this comment

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

I think the least destructive option should always be the default, so I vote for force=False. The first thing I always do after digging up a management command is checking the --help to see what options it has, and possibly reviewing the source code so I know exactly what it's going to do.

I also don't think an automated process is a good idea here. Having a silently watching daemon thingy that will swoop in and reingest everything when it feels like something has changed sounds creepy to me, not reassuring. If we make a change to the data format, we should expect to have to do some data migration, which I would always expect to be a manual process. I would not trust some automated script written in the distant past to handle that migration for me.

@jjnesbitt jjnesbitt merged commit a97b43e into master Apr 12, 2022
@jjnesbitt jjnesbitt deleted the update-zarr-ingestion branch April 12, 2022 16:30
@jjnesbitt jjnesbitt added the maintenance Action to maintain the system (neither a bugfix nor an enhancement) label Apr 12, 2022
@dandibot
Copy link
Member

🚀 PR was released in v0.2.8 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Action to maintain the system (neither a bugfix nor an enhancement) released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants