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

Better validate changelog #1519

Merged
merged 10 commits into from
Apr 9, 2025
Merged

Better validate changelog #1519

merged 10 commits into from
Apr 9, 2025

Conversation

waylan
Copy link
Member

@waylan waylan commented Apr 1, 2025

This is my attempt to ensure changelog validation catches issues so that deployment doesn't fail.

@waylan waylan added the release This PR is a prposed release. label Apr 1, 2025
@waylan
Copy link
Member Author

waylan commented Apr 1, 2025

As I suspected, the validation is not working correctly.

See this output.

This PR is labeled as a release and the latest version in Changleog is 3.5.2.

The actual changelog has entries for 3.6, 3.7, and unreleased. I would expect either 3.7 or unreleased returned for the latest version. Additionally, the status returned by the validator should be 'unreleased', which means we should not have received that message. The job should have failed with the message This PR is labeled as a release but the latest version in Changleog has status 'unreleased'. Either the changelog is invalid in some way that is confusing the validator and/or the validation tool has a bug.

This explains why deploy fails. Each time we deploy, the deploy action retrieves the version of the release and then attempts to use the validation tool to extract the release notes for that version to post as release notes here on GitHub. This action is supposed to ensure the changelog is valid before doing the release so that the release succeeds. Looks like we may need to switch tools.

@waylan
Copy link
Member Author

waylan commented Apr 1, 2025

Note, I have added a new label release. The thinking is that when we do a PR to bump the version, we set the release label on that PR which triggers the extra check. A PR with the release label should never have an unreleased entry in the changelog.

I have not done so yet, but we might want to also confirm that the actual version number of the release matches the version of the most recent entry in the changelog. In the deploy action we obtain the version number from the tag, but in a 'bump version' PR, the tag hasn't been set yet so we would need to retrieve the version from the code.

@waylan
Copy link
Member Author

waylan commented Apr 3, 2025

So apparently the changelog was so bad that the validator was simply not recognizing it properly.

First of all, Semantic Versioning requires all 3 of MAJOR, MINOR and PATCH to be set on every version. In other words, 3.0 is invalid; it must be 3.0.0. That explains why the validation tool was ignoring the most recent releases of 3.7 and 3.6 and returning 3.5.2 instead. It seems that the tool we are using ignores any second level headers which don't define proper versions. Another tool I tried locally requires all second level headers to be version entries and then reported invalid version errors.

Another issue is that the Keep a Changelog format apparently does not allow any headers within an entry. However, for every one of our MAJOR releases (and some MINORs) we have used headers (h4+) to break up and categorize our notes. When those were previously not raising any validation errors, I assumed this was allowed. However, it seems that the validation tool was skipping those sections due to the missing PATCH on the version number. As I have since fixed the versions, we are now getting validation errors for these entries (see this example). That will need to be fixed which will require a major refactoring of the changelog.

We could switch tools to avoid this issue from reappearing, but I don't think that is necessary. First of all, we have a section at the end of the changelog which links to the older (pre 3.0) changelong. We want that section to be ignored by the validator. A stricter validator would require us to change that in some way also. Also, if we add retrieval and comparison of the latest version (as I mentioned in a previous comment), that should avoid any issues with new entries not being properly recognized as an entry. However, we can revisit that after we get the changelog to validate properly.

@waylan
Copy link
Member Author

waylan commented Apr 3, 2025

It appears that the validation tool is not reporting unleased status as I expect. We may need to use a different tool after all.

@waylan
Copy link
Member Author

waylan commented Apr 4, 2025

After some experiments I have worked out that if the validator is run with version: Unreleased is returns the unreleased entry if there is one or errors if no such entry exists. I suspect we can use this in conjunction with the existence or not of the release label to determine if we have a changelog in the correct state.

@waylan
Copy link
Member Author

waylan commented Apr 4, 2025

Ah, I need to normalize 3.7 to 3.7.0. That should be the final piece. I think 🤷‍♂️

@waylan waylan removed the release This PR is a prposed release. label Apr 4, 2025
@waylan
Copy link
Member Author

waylan commented Apr 5, 2025

This seems to now be working as intended. The one thing I didn't expect is that adding or removing a label does not change things when a job is rerun. A new commit needs to be pushed.

@waylan waylan merged commit bd67d48 into Python-Markdown:master Apr 9, 2025
14 checks passed
@waylan waylan deleted the clvalidate branch April 9, 2025 16:23
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.

1 participant