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

Added file fields for a release's tarball and checksum #1801

Merged
merged 1 commit into from
Mar 13, 2025

Conversation

bmispelon
Copy link
Member

@bmispelon bmispelon commented Dec 7, 2024

This is in preparation for https://code.djangoproject.com/ticket/35980 which will likely change the naming scheme of build files (tarballs and checksums).

Here's a summary of the changes in this PR:

  • Added three new (optional) file fields to the Release model: tarball, wheel, and checksum
  • Wrote a data migration to populate the file fields based on existing data, so that existing https://www.djangoproject.com/m/releases/...` links keep working (see gist for data/scripts used).
  • Add some custom model validation to make the tarball field mandatory when a release is active (ie it has a release date). This is because the template in download.html assumes that all published/active releases have at least a tarball attached to them.
  • Added some fieldsets on the release admin form (see screenshot) to make the form a bit neater.
  • Added an accept attribute on the admin form file widgets to make it easier to pick the right file for each
  • Removed all hardcoded filenames for tarballs and checksums across the site
  • Removed custom logic to compute release file names based on date/version number (the actual file names from the Release model can now be used)
Screenshots

Screenshot 2025-02-15 at 13-26-58 5 2 Change release Django site admin

I've also prepared a companion PR to the documentation of the release process over at django/django#19178

@bmispelon bmispelon self-assigned this Dec 7, 2024
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Overall looks good! Added a comment, but I think it makes sense.

@nessita
Copy link
Contributor

nessita commented Dec 9, 2024

On a second look, and after some local testing, I wonder why we don't spare the new fields and just make them a property? That seems simpler and that it scales better...

@nessita
Copy link
Contributor

nessita commented Dec 27, 2024

@bmispelon Hi! Do you think you'd have any availability to continue your work on this?

@nessita
Copy link
Contributor

nessita commented Feb 3, 2025

@bmispelon Hi! Could you please help me understand whether you'd have availability to keep on this? Otherwise, no worries, I can take it: setuptools is going to be released soon with the fix so we are planning next steps to push this and other changes around it forward. Thank you!

@bmispelon
Copy link
Member Author

@bmispelon Hi! Could you please help me understand whether you'd have availability to keep on this? Otherwise, no worries, I can take it: setuptools is going to be released soon with the fix so we are planning next steps to push this and other changes around it forward. Thank you!

I've picked this up again, thanks for the ping. I've rebased the branch, fixed the CI, and updated the first comment with a list of what's left for me to do.
I'm hoping to be done this week 🤞🏻 (if not, I'll ping you and we can figure out if it makes sense for you to take over)

@bmispelon
Copy link
Member Author

@nessita I think this is ready for a re-review. I've taken your advice and simplified the migration a bit so that only exceptions to the general naming rule are listed explicitly. I've always made a few quality-of-life improvements to the admin form (fieldsets, custom validation) but if you think that's going to get in the way of your work please let me know.

@bmispelon bmispelon force-pushed the pep625 branch 2 times, most recently from 3a3e863 to b43854b Compare February 15, 2025 16:40
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thank you @bmispelon for this work, I have applied my suggestions locally and I have made other changes based on what we talked during the DSF Office Hours (pushing that now).

I haven't fixed the tests, would you have any availability to fix them? 🌟

Comment on lines 13 to 14
for fieldname, accept in _ARTIFACT_FILE_EXTENSIONS.items():
self.fields[fieldname].widget.attrs["accept"] = accept
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this and put it in the model using a FileExtensionValidator. I will add a suggest there.

Copy link
Member Author

Choose a reason for hiding this comment

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

The accept attribute on the <input> also had the benefit of filtering the list of visible files in the file picker, it's not just about validation (which should be done server side, I agree).

@bmispelon
Copy link
Member Author

I've fixed the tests and pushed a few changes:

  • Removed the validators: the validation is now all handled in the clean() (I also added a lot more tests)
  • Removed the explicit call to full_clean() in save(). This made some unrelated tests fail and is not really needed because the modelforms from the admin will go through that validation: https://github.com/django/django/blob/main/django/forms/models.py#L498

I think we should restore the <input accepts=...> changes for the reasons I wrote in the comment.
Same with the {% if release.checksum %": personally I'd keep it.

Otherwise, I like the changes you made to keep the case of the uploaded artifact, I think it makes sense.

@nessita nessita force-pushed the pep625 branch 2 times, most recently from ca24abb to ac20809 Compare March 11, 2025 20:35
@nessita
Copy link
Contributor

nessita commented Mar 12, 2025

@bmispelon I pushed an update where the ReleaseManager.active method was renamed to .published, taking into consideration the is_active flag. There are no other usages of .active in the code:

$ grep -nR "\.active" --exclude-dir=static_root --exclude-dir=static
blog/models.py:32:        return self.active().filter(pub_date__lte=timezone.now())
djangoproject/scss/_style.scss:377:        &.active {
djangoproject/scss/_style.scss:527:        &.active {
djangoproject/scss/_style.scss:539:        &.active {
djangoproject/scss/_style.scss:577:            &.active a {
djangoproject/scss/_style.scss:1880:    &.active {
djangoproject/scss/_style.scss:1887:            &.active {
djangoproject/scss/_style.scss:1942:        li.active .collapsing-content {
djangoproject/scss/_style.scss:3079:        &.active {

Copy link
Contributor

@sarahboyce sarahboyce 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!

@nessita nessita force-pushed the pep625 branch 2 times, most recently from 8c2b2ef to 3a1456b Compare March 12, 2025 18:59
@nessita
Copy link
Contributor

nessita commented Mar 12, 2025

Just a thought, it might be worth changing this in a separate PR/deployment so that we know the data has been migrated successfully

@sarahboyce This is a good idea and I have reverted this change in a separated commit that we can later squash. The rest of the feedback was addressed. Thank you!

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

This looks great to me!

Three new file fields (tarball, wheel, checksum) were added, making
the link generation to these files more robust to changes like the
one mandated by PEP625 (changing of file case).

A new is_active boolean field was also added, making it easier
to start work on a release without it being public.
@bmispelon bmispelon merged commit 3032202 into django:main Mar 13, 2025
4 checks passed
@bmispelon
Copy link
Member Author

Now we just need a fresh new Django version to test things out. Wanna roll out an unplanned release just for fun? 🙊

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.

3 participants