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 published field to Release #17257

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

DarkaMaul
Copy link
Contributor

@DarkaMaul DarkaMaul commented Dec 9, 2024

This PR introduces the following changes :

  • a new boolean published field to the Release model
  • a migration to update every existing releases and set their published value to be the creation time

Part of #17230

Some notes:

I dislike how I changed the default Release 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 the published field is set.

The problem boils down to not setting a value for publish 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 explicit null value ( using forcing-null-on-a-column-with-a-default )

The new implementation uses a boolean field.

@DarkaMaul DarkaMaul requested a review from a team as a code owner December 9, 2024 14:00
@DarkaMaul DarkaMaul changed the title Dm/published date Add published field to Release Dec 9, 2024
@di
Copy link
Member

di commented Dec 9, 2024

Should this follow the same pattern we have for yanking, where there are yanking events with timestamps, and we have a yanked field on the model that is a boolean instead?

@DarkaMaul
Copy link
Contributor Author

Should this follow the same pattern we have for yanking, where there are yanking events with timestamps, and we have a yanked field on the model that is a boolean instead?

I like the idea as it would also not modify how new Releases are created.

Copy link
Member

@woodruffw woodruffw left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants