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 JSON-based changes to auto-generate the changelog. #918

Merged
merged 1 commit into from
Jul 10, 2022

Conversation

Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Jul 6, 2022

Solves merge conflicts between changelogs, by using sorting and type-based merging for the changelog from a series of JSON files.

Closes #662.

@Alexhuszagh Alexhuszagh added the meta issues/PRs related to the maintenance of the crate. label Jul 6, 2022
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 6, 2022

Currently this needs a CHANGELOG entry and it will need a script to automerge and delete the entries on creating a release, or at least auto-merge them in on a PR or something. But the basic logic is here. The diff between the CHANGELOG.md and CHANGELOG.md.draft for this PR currently is:

34a35,38
>
> ### Internal
>
> - #918 - use JSON-based files to autogenerate CHANGELOG.md

Completed:

  • Auto-generate changelog from a list of changes.
  • Script to auto-merge on release is done via `pre-release-hook.
  • All multiple changes per PR.
  • Add template.json.
  • Delete all JSON entries in .changes on the release build.
  • Add validator that takes a list of files, with useful error messages when the JSON or changelog fails to parse.
  • Add github actions to verify changelog entries.

@Alexhuszagh Alexhuszagh force-pushed the changesets branch 2 times, most recently from 1a751d2 to 849d0f5 Compare July 6, 2022 20:57
@Alexhuszagh
Copy link
Contributor Author

This action should be able to resolve our changelog check, btw.

We also, if the draft is not used, need to delete every JSON file in .changes.

@Emilgardis
Copy link
Member

Emilgardis commented Jul 6, 2022

could this handle the situations when there are multiple prs before release doing the same thing, i.e entries adjusting the same changelog entry?

Don't want the changelog to be a per-pr changelog, for that we could just use autogenerated changelogs

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 6, 2022

could this handle the situations when there are multiple prs before release doing the same thing, i.e entries adjusting the same changelog entry?

Don't want the changelog to be a per-pr changelog, for that we could just use autogenerated changelogs

The way to modify the entries pre-release would be to just modify that JSON file I believe. I've also added it so a single PR can modify multiple different changes: you can pass an object or array.

[
    {
        "description": "sample 1, sample 2",
        "type": "added"
    },
    {
        "description": "sample 3, sample 4",
        "type": "added"
    },
    {
        "description": "some internal stuff",
        "issues": [662],
        "type": "internal"
    }
]

Or

{
    "description": "use JSON-based files to autogenerate CHANGELOG.md",
    "issues": [662],
    "type": "internal"
}

This will get merged to:

### Added

- #919 - sample 1, sample 2
- #919 - sample 3, sample 4

### Internal

- #919 - some internal stuff
- #918 - use JSON-based files to autogenerate CHANGELOG.md

The entries are sorted by PR number, so they will automatically sort in descending order.

To modify an entry prior to release, just modify the JSON entry. It handles any number of changes, and doesn't use the changes until release (when the pre-release hook is used). However, you can update the CHANGELOG at any time, and the draft is meant to all you to verify it's working properly.

@Alexhuszagh Alexhuszagh force-pushed the changesets branch 22 times, most recently from 70f8e86 to 862fc45 Compare July 7, 2022 00:14
@Alexhuszagh Alexhuszagh removed the no changelog A valid PR without changelog (no-changelog) label Jul 7, 2022
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 7, 2022

Ok I've updated the Github Actions for the changelog check to:

  • Check if any file has changed in the .changes/*.json directory (added, modified, or removed).
  • Validate every JSON file by parsing using our serde deserializer, if added or modified.
  • Validate the CHANGELOG.md file (but don't look for changes).
  • Added a pre-release hook that auto-generates the update CHANGELOG.md

This should do everything. How it works is quite simple:

  1. Parses every entry in .changes/*.json if they are not template files.
  2. The filename is the PR number, the contents are the CHANGELOG data (description and type are required).
  3. These are then added to vecs by each type (Added, Changed, Fixed, Internal, Removed).
  4. The CHANGELOG.md file is read, and the contents between <!-- next-header --> and <!-- last-header --> are read.
  5. Each section that starts with a ### is used to identify the changelog type, and the PR number and description our extracted from each entry starting with - .
  6. These are then added to different vecs by each type (Added, Changed, Fixed, Internal, Removed).
  7. These two sets of vecs are merged and sorted by PR number.
  8. These are then formatted to string, and the header (before <!-- next-header -->) and the footer (after <!-- last-header -->) are printed verbatim.

This means there will never be conflicts: multiple PRs can have the same types (numerous added types, etc.), can have one or more types (see the example), and since they are merged into a vector and sorted by PR number, the result will always be in descending order and look nice.

If you would like to verify this, rename the template files to 920.json and 925.json (or whatever) and run:

$ cargo xtask build-changelog --draft

The --draft is to ensure this a dry-run: it will not overwrite CHANGELOG.md, nor delete the JSON files in .changes. You can then compare the two entries. I've also tested the changelog action and it finally works how I want it to: it runs the validator script on all added or modified files that match .changes/*.json if no changelog is not set, using the basenames. If no files were changed and no changelog is not set, then it errors out.

@Alexhuszagh Alexhuszagh marked this pull request as ready for review July 7, 2022 00:44
@Alexhuszagh Alexhuszagh requested a review from a team as a code owner July 7, 2022 00:44
Copy link
Member

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

The ./.github/actions/cargo-publish action needs to be updated to generate the changelog on pre-release.

There's also an issue with pre-releases, should pre-releases delete the changeset files or not?

.changes/README.md Outdated Show resolved Hide resolved
.changes/template_array.json Outdated Show resolved Hide resolved
.changes/README.md Outdated Show resolved Hide resolved
.changes/README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
.changes/README.md Outdated Show resolved Hide resolved
.changes/README.md Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
xtask/src/changelog.rs Outdated Show resolved Hide resolved
@Alexhuszagh
Copy link
Contributor Author

I think it's probably a good idea, in retrospect, not delete the .changes, but to move them to a dedicated subdirectory based on the release number? Then the default is to make a draft, and if the release is present, move all files over and then update the CHANGELOG.

@Alexhuszagh
Copy link
Contributor Author

I believe the only thing left is adding the cargo-publish.

You can test the parsing and generating of the CHANGELOG via cargo test --workspace, which auto-runs the tests to parse and format the documents, as well as parse and validate the deserializers from JSON. It tests having no PR, multiple PRs, multiple issues, breaking changes, and no breaking changes. For example, the files it tests that parse correctly are:

https://github.com/Alexhuszagh/cross/blob/c20f3f0855c003559ec448a3ea4b6081ee6bdbe9/xtask/src/changelog.rs#L821-L852

And it checks that the output is reasonable, say, for here:

https://github.com/Alexhuszagh/cross/blob/c20f3f0855c003559ec448a3ea4b6081ee6bdbe9/xtask/src/changelog.rs#L799-L816

However, cargo release isn't working correctly yet, since the hook is run after the pre-release statements are made, which I need to fix.

@Emilgardis
Copy link
Member

Emilgardis commented Jul 7, 2022

This is great, however I feel like it should actually probably be part of its own binary/crate.

Maybe something for https://github.com/crate-ci/ ?

@Alexhuszagh Alexhuszagh marked this pull request as draft July 7, 2022 23:39
@Alexhuszagh Alexhuszagh force-pushed the changesets branch 2 times, most recently from 4f1cdba to b89a186 Compare July 9, 2022 22:45
@Alexhuszagh Alexhuszagh marked this pull request as ready for review July 9, 2022 22:46
@Alexhuszagh
Copy link
Contributor Author

This now handles cargo-release properly (well, it should, assuming the pre-release replacements are actually written to file prior to the hook getting invoked, which I believe they are). The only issue right now is it also makes the changes if the release is a release-candidate. I believe we shouldn't update the changelog version in that case, just add the changes but keep them in an Unreleased state.

You can test it for yourself running cargo release patch --verbose.

@Alexhuszagh
Copy link
Contributor Author

This finally handles pre-releases correctly, and it will behave as follows:

When using a pre-release, it will update CHANGELOG.md but keep everything in an Unreleased section. The changes will be moved to .changes/{major}.{minor}.{patch}, so they will be in the same directory as any additional changes prior to the release.

If a release occurs, it will change the unreleased section from:

## [Unreleased] - ReleaseDate

### Internal

- #918 - use JSON-based files to autogenerate CHANGELOG.md

to:

## [Unreleased] - ReleaseDate

## [v0.2.4] - 2022-07-10

### Internal

- #918 - use JSON-based files to autogenerate CHANGELOG.md

@Emilgardis
Copy link
Member

looks good! There's something wrong with the behaviour though.

cross on changesets [$] is 📦 v0.2.3 via 🦀 v1.61.0 
❯ cargo release 0.3.0 --execute
[2022-07-10T12:54:01Z WARN  cargo_release::git] Push target `origin/changesets` doesn't exist
Release cross 0.3.0? [y/N] 
y
[2022-07-10T12:54:03Z INFO  cargo_release::release] Update cross to version 0.3.0
   Compiling cross v0.3.0 (/Users/emil/workspace/cross)
   Compiling xtask v0.0.0-dev.0 (/Users/emil/workspace/cross/xtask)
    Finished dev [unoptimized + debuginfo] target(s) in 2.90s
     Running `target/debug/xtask build-changelog`
Building the changelog.
Running rustfmt and clipp sy checks.
+ flags=(--all-features --all-targets --workspace)
+ cargo fmt -- --check
+ cargo +nightly
+ cargo +nightly clippy --all-features --all-targets --workspace -- --deny warnings
    Blocking waiting for file lock on build directory
   Compiling cross v0.3.0 (/Users/emil/workspace/cross)
    Checking xtask v0.0.0-dev.0 (/Users/emil/workspace/cross/xtask)
    Finished dev [unoptimized + debuginfo] target(s) in 4.60s
[changesets cf3b8ee] release version 0.3.0
 4 files changed, 11 insertions(+), 8 deletions(-)
 delete mode 100644 .changes/918.json

cross on changesets [$?] is 📦 v0.3.0 via 🦀 v1.61.0 took 11s 
❯ git status
On branch changesets
Untracked files:
  (use "git add <file>..." to include in what will be committed)
        .changes/0.3.0/

nothing added to commit but untracked files present (use "git add" to track)

the 0.3.0 folder does not get added. Also, should we really keep these files after release?

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 10, 2022

the 0.3.0 folder does not get added. Also, should we really keep these files after release?

I've changed from renaming those files to just deleting them. This is ready for review. The only additional thing I could think of would be supporting YAML and TOML as well, since we practically do that already, and already have all the dependencies. YAML might be nice, since it looks like:

description: "sample description for a PR adding one CHANGELOG entry."
issues:
    - 437
type: "fixed"

And TOML would be:

description = "sample description for a PR adding one CHANGELOG entry."
issues = [437]
type = "fixed"

@Emilgardis
Copy link
Member

I think json is fine for this

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

lgtm!

fix the trailing folder and we can start to use this!

xtask/src/changelog.rs Outdated Show resolved Hide resolved
Solves merge conflicts between changelogs, by using sorting and
type-based merging for the changelog from a series of JSON files. This
allows conflict-free generation of a changelog using JSON-based changes,
stored in `.changes`.

Each entry can be object or array-based: the array must contain objects,
where each object is a different change. The entries are added to the
changelog, in sorted order, and the validity of the changelog is
verified.

To ensure fidelity of the changelog changes, the `no changelog` flag or
at least 1 change (either removed, or added/dified and almovidated) for
a JSON file under `.changes` must occur.

A `pre-release-hook` has been added so all these changes are automerged
into the CHANGELOG prior to release.
@Alexhuszagh
Copy link
Contributor Author

bors r=Emilgardis

@bors
Copy link
Contributor

bors bot commented Jul 10, 2022

Build succeeded:

@bors bors bot merged commit c6bedf2 into cross-rs:main Jul 10, 2022
@Alexhuszagh Alexhuszagh deleted the changesets branch July 10, 2022 23:53
@Emilgardis Emilgardis added this to the v0.3.0 milestone Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta issues/PRs related to the maintenance of the crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minify impact of CHANGELOG.md enforcement
2 participants