-
Notifications
You must be signed in to change notification settings - Fork 980
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 published field to Release #17257
base: main
Are you sure you want to change the base?
Conversation
Should this follow the same pattern we have for yanking, where there are yanking events with timestamps, and we have a |
I like the idea as it would also not modify how new Releases are created. |
f4161f6
to
60de6e8
Compare
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.
LGTM, nice work @DarkaMaul!
Given that published
is currently invariant as True
, this shouldn't cause any observable index changes. However, for the follow-up where we make it controllable, I think we should add some integration tests to make sure the index responses are what we expect.
This PR introduces the following changes :
published
field to the Release modela migration to update every existing releases and set their published value to be the creation timePart of #17230
Some notes:
I dislike how I changed the defaultRelease
creation call to force the publish time to be set to a timestamp, but I did not find a better solution. This created a new quirk: every time someone wants to create a new Release, they must make sure thepublished
field is set.The problem boils down to not setting a value forpublish
at the release creation, which would either semantically mean the time is not set (the solution I chose) or use a default value (e.g., the current timestamp). When choosing the latter case, we would need a placeholder value on the Python side (e.g.,published=False
) and to convert this placeholder to an explicitnull
value ( using forcing-null-on-a-column-with-a-default )The new implementation uses a boolean field.