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

Reorder overwritten #130

Merged
merged 4 commits into from
Jun 4, 2024
Merged

Reorder overwritten #130

merged 4 commits into from
Jun 4, 2024

Conversation

stuartmcalpine
Copy link
Collaborator

Move the tagging of is_overwritten=True for the previous datasets to after the copy was successful

Copy link
Collaborator

@JoanneBogart JoanneBogart left a comment

Choose a reason for hiding this comment

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

The change you're making looks fine, but I noticed something else nearby which maybe should be done differently. It may belong in a separate PR.
For datasets with dataset_location != "dataregistry" valid_status is left at 0. It's true there is no valid copy of the data under the control of the data registry, but I think we should distinguish such entries from those which are truly failed - either incomplete db entry, failed copy of data, or no data present where it should be when old_location was None. I think we need 3 possible values for valid_status, e.g. another bit in status to keep track of whether valid data exists or not. It would never be set for entries with dataset_location != "dataregistry", but it wouldn't mean anything was wrong in that case either. Or, alternatively, just use the combination of status and dataset_location to figure out what the valid bit means.

Either way this could require changes elsewhere in the code. If you'd rather handle that in a separate PR I can go ahead and approve this one as it stands.

@stuartmcalpine
Copy link
Collaborator Author

It looks like the valid_status variable is something left over from before, it's never actually used.

All datasets regardless of type are giving a valid=True bit if they have a successful entry. I think that's ok, they are all "valid". They can still be distinguished using the location_type column.

I've removed the left over valid_status variable.

Copy link
Collaborator

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

@stuartmcalpine stuartmcalpine merged commit deda722 into main Jun 4, 2024
20 checks passed
@stuartmcalpine stuartmcalpine deleted the u/stuart/reorder_overwrite branch June 4, 2024 14:00
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.

None yet

2 participants