-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[23.2] Set metadata states on dataset association, not dataset #17474
[23.2] Set metadata states on dataset association, not dataset #17474
Conversation
This is a minimal fix, we should also split the states between valid dataset states and valid dataset instance states, and have the state setter dispatch to the correct table if metadata states are being set. |
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.
Thanks!
Existing servers may want to do something like:
|
I think |
|
Thanks for checking, I didn't know this! |
I doubt anyone instinctively know when to use `._state`, `.state`, `.raw_set_dataset_state` or `.set_dataset_state` and I think that was part of what led to this bug.
864051a
to
513ca74
Compare
sa_session.add(self.dataset) | ||
self.dataset.state = state | ||
|
||
def set_metadata_succces_state(self): |
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.
Late to the party, but I guess that should have been set_metadata_success_state
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.
Oops, fixed in #17481
Fixes an issue that @natefoo reported, where other users could get the dataset state to change to FAILED_METADATA on the source.
How to test the changes?
(Select all options that apply)
License