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 changelog #4448

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add changelog #4448

wants to merge 2 commits into from

Conversation

aaruni96
Copy link
Member

This PR adds a changelog file to Oscar. The changelog should include all noteworthy changes to Oscar. This also adds a changelog CI check to every PR, but this will not be a required check, just a reminder.

The template is borrowed from https://keepachangelog.com/en/1.1.0/.

Addresses #4431 .

@fingolfin
Copy link
Member

Thank you for making a first draft as basis for discussions.

My two cents: I am not sure if having a CI check which flags PRs that don't modify CHANGELOG.md is the best way forward: Editing CHANGELOG.md in many PRs will result in many conflicts by multiple PRs editing that file.

Moreover, it leaves open the question who really is responsible for adding entries to CHANGELOG.md

For sake of discussion, I'll describe an alternative model used by GAP at https://github.com/gap-system/gap which revolves around labels for PRs. In GAP we use these labels:

  • release notes: not needed: added to PRs which don't need to be mentioned in release notes (e.g. typo fixes, changes to CI setup, fixing regressions for issues that were never in a release etc. pp.)
  • release notes: use title: the title of the PR is suitable as text for the release notes / CHANGELOG
  • release notes: to be added: an entry in release notes should be added but it will require manual intervention (e.g. its a standout feature that should be mention with a longer text)
  • (a few more but they are not important for the description)

A couple other labels are used to indicate in which category the release notes fit (so in the context of this PR these might correspond to "Added", "Changed", "Deprecated", "Removed", etc.; but one could also use other categories, either instead or in addition to ehse, for example based on feature domain: "Algebraic Geometry", "Polyhedral Geometry", "Lie Theory", "Number theory", "Group Theory", etc.)

"Ordinary" contributors don't have to interact with these at all (indeed if you are not a project member, you can't set labels). Instead experienced developers may add these labels to PRs. A key point is that this can happen at any time: during creation of a PR, while it is open, or even after it is merged.

In GAP we then have a script which gets as input the new version and which queries all PRs merged since the last release, and based on the labels updates the release notes / changelog. PRs which don't have the required label are put into a separate section "uncategorized".

Then someone tasked with updating the release notes (e.g. release managers) can occasionally run this script and look into PRs w/o release notes labels, and deal with them: e.g. by requesting that the PR author provide a succinct summary suitable for the release notes, which then is put into the PR title and a label release notes: use title is added.

In GAP this works quite well because it avoids merged conflicts; and decouples adding new work from writing the changelog entry, which reduces the barrier for contribution; while making it possible to still keep track of what's changed in a systematic way so nothing is accidentally "forgotten".

Of course in the end someone has to write a summary for a PR; that is additional work we can't quite eliminate if we want useful release notes. But we can still strive to make it as painless as possible.

@fingolfin
Copy link
Member

We discussed this in triage today and agreed to try my suggestion of working with labels (of course we can and will evaluate & revise this based on how it works in practice).

As such the workflow being added here should be removed.

One good suggestion by @HereAround was to set up another CI job which runs the scripting updating CHANGELOG.md regularly, creating resp. updating a draft PR.

I also am not yet sure if the suggested categories in CHANGELOG.md are what we want in the end, but I am happy to start with these and we figure it out as we go.

In the meantime, @aaruni96 can you please make similar PRs for AA and Nemo?

@joschmitt
Copy link
Member

Could someone also add instructions to the developer docs? I think I understand how the system is supposed to work, but I always like to read up on things :)
(Of course, the instructions can change whenever the process changes.)

@aaruni96
Copy link
Member Author

Removed the changelog workflow.

But the labels got me thinking, should we consider a default PR template, which adds a label like release-notes: TBD, so that every PR requires actively changing the release notes label if we decide they're not required ?

Documentation for PR template : https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository

@fingolfin
Copy link
Member

Yes, we definitely need to document in the dev docs how the changelog update workflow is supposed to work, and how to use those labels. Perhaps the description we have for GAP can be used as a starting point.

Regarding PR template and labels: we don't need a release-notes: TBD label, the absence of any release-notes: * label is sufficient (at least for the scripts I use, and also from the web interface it is easy enough to query for PRs missing certain labels).

But a template still would be a good idea, namely to suggest that people should provide a release note draft (and how), and possible info on how to set labels (but phrased in a way that makes it clear that this only applies to people who actually can set labels; i.e. don't confuse newbies who may frantically search for how they can add labels when they simply can't).

E.g. this is what we use for GAP: https://github.com/gap-system/gap/blob/master/.github/pull_request_template.md

The Python script we use in GAP for updating release notes is here: https://github.com/gap-system/gap/blob/master/dev/releases/release_notes.py

It uses the gh command line tool in one spot to query for relevant PRs. If desired this could also be rewritten to e.g. use PyGitHub, but beware that it uses an (back when I added it rather important) optimization to ensure the query only returns the data we need, not the entire records for each PR which can be huge (meaning that one hits the rate limiter more quickly, which is easy to do when executing these queries not logged in. That was the other reason to use gh: because there the "how to authenticate with GitHub" problem is solved very well and I use gh anyway...).

Aaanyway, it should be possible to relatively quickly modify the script for us. And then improve it: right now it produces a new file, and then the release manager (for GAP that is me) inserts it into the real changelog manually. But this could easily be automated, too, to insert the new text into the "right" spot.

Comment on lines +22 to +23

### Security
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Security

I don't think this is a category that is relevant for us... or if it ever becomes, we can add it :-)

On the other hand, I wonder how breaking changes fit into this scheme: right now "breaking" could appear under "changed" or "removed" (and arguably even under "fixed"). But we should make breaking changes prominent. I see two options:

  1. Add another section "Breaking" (I'd put it first) and put them all in there, no matter whether they remove or change something
  2. Use these categories but then always put Breaking: in front of breaking changes

(As @thofma pointed out for OSCAR this shouldn't be relevant for some time, but certainly for AA and Nemo. But I am asking this here in the hopes that more people will see it this way).

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

Successfully merging this pull request may close these issues.

4 participants