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

use towncrier to handle changelog entries #8671

Merged

Conversation

zacharyburnett
Copy link
Collaborator

@zacharyburnett zacharyburnett commented Jul 24, 2024

related to #8670

using towncrier to handle changelog entries will

  • drastically reduce (or even eliminate) branch conflicts in CHANGES.rst we currently experience with PRs
  • keep the sections in CHANGES.rst consistent and eliminate duplicate sections
  • allow automatic links to be generated in the changelog entries

image

towncrier expects "news fragment" files (text files in the changes/ directory with filenames in the format <PR#>.<changetype>.rst, i.e. for this PR it would be changes/8671.docs.rst). See docs at https://towncrier.readthedocs.io/en/latest/tutorial.html#creating-news-fragments

when ready to make a release, run towncrier build to ingest the news fragments and generate a changelog section in CHANGES.rst with all the new change log entries for that release (this clears the changes/ directory of all news fragment files). This step should either be done before making a release, or could probably be added to a GitHub workflow triggered on release (to insert a commit and remake the tag).

After merging this PR the Release Process wiki page will need to be updated to include a step to run towncrier build instead of manually editing CHANGES.rst

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly

@zacharyburnett zacharyburnett added this to the Future milestone Jul 24, 2024
@zacharyburnett zacharyburnett self-assigned this Jul 24, 2024
@github-actions github-actions bot added installation automation Continuous Integration (CI) and testing automation tools labels Jul 24, 2024
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.86%. Comparing base (7683808) to head (e739136).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8671      +/-   ##
==========================================
+ Coverage   61.81%   61.86%   +0.04%     
==========================================
  Files         377      377              
  Lines       38915    38911       -4     
==========================================
+ Hits        24056    24071      +15     
+ Misses      14859    14840      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zacharyburnett zacharyburnett marked this pull request as ready for review July 24, 2024 16:14
@zacharyburnett zacharyburnett requested a review from a team as a code owner July 24, 2024 16:14
@zacharyburnett zacharyburnett requested a review from nden July 24, 2024 16:28
@zacharyburnett
Copy link
Collaborator Author

as @braingram pointed out in tagup this morning, using towncrier also has the advantage of reducing merge conflicts in CHANGES.rst when multiple PRs change the same changelog section, as well as preventing the edge case of a long-standing PR putting its changelog entry into an old version section

@braingram braingram mentioned this pull request Aug 6, 2024
8 tasks
@braingram
Copy link
Collaborator

Would this require adding a subdirectory and pyproject.toml entry for each possible heading? The current changelog has many headings:

jwst/CHANGES.rst

Lines 4 to 5 in 1a45e34

align_refs
----------

and looking at astropy's use of towncrier it has a subdirectory for each section:
https://github.com/astropy/astropy/tree/main/docs/changes/config
and a corresponding entry in pyproject.toml

Also, is it required to manually run towncrier for each build or is this something the CI can handle?

@zacharyburnett
Copy link
Collaborator Author

Would this require adding a subdirectory and pyproject.toml entry for each possible heading? The current changelog has many headings:

jwst/CHANGES.rst

Lines 4 to 5 in 1a45e34

align_refs
----------

and looking at astropy's use of towncrier it has a subdirectory for each section:
https://github.com/astropy/astropy/tree/main/docs/changes/config
and a corresponding entry in pyproject.toml

If we want to keep the current headings, then yes. However, I also wanted to discuss whether it would be more useful for the user to condense the headings to the defaults here; do the current module-based headings make the changelog more or less confusing?

Also, is it required to manually run towncrier for each build or is this something the CI can handle?

This could be done with a release action, which would have to make a commit freezing the changelog

@braingram
Copy link
Collaborator

If we want to keep the current headings, then yes. However, I also wanted to discuss whether it would be more useful for the user to condense the headings to the defaults here; do the current module-based headings make the changelog more or less confusing?

I don't have an informed opinion here so I'll keep quiet :)

@zacharyburnett
Copy link
Collaborator Author

If we want to keep the current headings, then yes. However, I also wanted to discuss whether it would be more useful for the user to condense the headings to the defaults here; do the current module-based headings make the changelog more or less confusing?

I don't have an informed opinion here so I'll keep quiet :)

calling @nden; should we keep the current changelog headings? I can add them here

@tapastro
Copy link
Contributor

There are use cases for the changelog during development - it's convenient to have a single location to read all updates since the last release. Is there another option besides only building the changelog on release under towncrier?

My 2c would be that the subheadings are very useful and should remain, fwiw.

@zacharyburnett
Copy link
Collaborator Author

zacharyburnett commented Aug 15, 2024

There are use cases for the changelog during development - it's convenient to have a single location to read all updates since the last release. Is there another option besides only building the changelog on release under towncrier?

The downside, however, is that the developer must manually update the changelog with every release, and changelog entries might be accidentally inserted into old changelog sections. The changelog/ directory will still have all of the updates since the last release (the files in it are cleared by the towncrier action when creating a new changelog section). However, this is not as convenient as a single file, rather several files in a single directory. EDIT: it looks like you can use towncrier build --draft to print the formatted changelog from the current news fragments

My 2c would be that the subheadings are very useful and should remain, fwiw.

Alright, I'll add them back

@zacharyburnett zacharyburnett changed the title use towncrier to automatically create changelog entries handle changelog entries with towncrier Aug 16, 2024
@zacharyburnett zacharyburnett force-pushed the changelog/towncrier branch 3 times, most recently from a901171 to b913409 Compare August 19, 2024 17:31
@zacharyburnett zacharyburnett marked this pull request as draft August 19, 2024 17:53
@zacharyburnett zacharyburnett force-pushed the changelog/towncrier branch 3 times, most recently from cd95b10 to 8e31c64 Compare August 19, 2024 20:02
@zacharyburnett zacharyburnett marked this pull request as ready for review August 19, 2024 20:02
@zacharyburnett
Copy link
Collaborator Author

zacharyburnett commented Aug 22, 2024

this will pair nicely with #8716

@melanieclarke
Copy link
Collaborator

I really like this proposal in principle. I think everyone will be happier not having to de-conflict the change log all the time. But I have a few opinions about the proposed formatting:

  • I'd prefer alphabetical by step name. It's easier to find things that way, without thinking too hard about where the step might appear in a larger context. I do like the note about which pipelines the step appears in, though!
  • The additional formatting for the subject headings and the PR links makes the file a little harder to read in plain text. The links to PRs are useful enough in the built version that I think they're worth keeping. But I'd prefer not to put the "``" around all the step names: they are visually distracting in both the plain text and built version of the docs.

Also, I'd like to propose that we hold off on transitioning to this system until after the next build -- development ends in just a couple weeks anyway. That way you don't have to try to keep the fragments in line with current development, and we can start fresh with a clean change log for the next build.

@zacharyburnett
Copy link
Collaborator Author

  • I'd prefer alphabetical by step name. It's easier to find things that way, without thinking too hard about where the step might appear in a larger context. I do like the note about which pipelines the step appears in, though!

I can do that, sure

  • The additional formatting for the subject headings and the PR links makes the file a little harder to read in plain text. The links to PRs are useful enough in the built version that I think they're worth keeping. But I'd prefer not to put the "``" around all the step names: they are visually distracting in both the plain text and built version of the docs.

I wanted to emphasize that this is the module name, but you're right it does make it more visually complex

Also, I'd like to propose that we hold off on transitioning to this system until after the next build -- development ends in just a couple weeks anyway. That way you don't have to try to keep the fragments in line with current development, and we can start fresh with a clean change log for the next build.

That sounds great to me! I'll wait until the next build

Copy link
Contributor

@tapastro tapastro 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! Just a couple of requested additions to fragment types.

@melanieclarke
Copy link
Collaborator

@zacharyburnett - can we also remove the back ticks around the pipeline names in the fragment types?

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

LGTM, when Tyler is happy. Thanks for all the updates -- looking forward to getting this in and never de-conflicting the change log again!

@tapastro tapastro self-requested a review September 23, 2024 16:44
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation Continuous Integration (CI) and testing automation tools documentation installation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants