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

ci: autogenerate changelog #330

Merged
merged 17 commits into from
Jul 26, 2024
Merged

ci: autogenerate changelog #330

merged 17 commits into from
Jul 26, 2024

Conversation

janbuchar
Copy link
Collaborator

@janbuchar janbuchar added the t-tooling Issues with this label are in the ownership of the tooling team. label Jul 19, 2024
@janbuchar janbuchar requested review from vdusek and B4nan July 19, 2024 15:09
@janbuchar janbuchar marked this pull request as draft July 19, 2024 15:15
@janbuchar
Copy link
Collaborator Author

Made this a draft, I'll try to update this so that it works with workflow_dispatch

@janbuchar janbuchar marked this pull request as ready for review July 23, 2024 21:03
@janbuchar
Copy link
Collaborator Author

@vdusek @B4nan I haven't run this yet, I imagine there will be quite some hotfixing. Also version bumps are still quite spaghettific.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • The release artifacts seem bordeline useless - I hope nobody installs packages by downloading .whl files from github. If someone tried, dependency management would be hell. Are there any points against dropping them?
  • It is nice that we make tags for beta releases, but shouldn't we make github pre-releases instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Github pre-releases can swamp the release list page - see https://github.com/vercel/next.js/releases. You can use the prerelease:false search filter, but still.

Copy link
Member

Choose a reason for hiding this comment

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

I don't like to pollute anything else than the package manager for prereleases, I would remove the git tags as well (I did in some repositories already, e.g. in the JS monorepos we maintain), since I can't justify adding tags to every single commit to master.

Copy link
Collaborator Author

@janbuchar janbuchar Jul 24, 2024

Choose a reason for hiding this comment

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

Sure, just removing the git tags is also an option. We'd lose a bit of traceability (what changed between those betas?), but if we keep releasing often, nobody will care 🙂

Copy link
Member

Choose a reason for hiding this comment

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

If every commit to master is a beta, it should be pretty clear what changed between those betas.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is, if you have 0.42.2b73 installed, how do you look up what changed between that and the current 0.42.2b91 if there are no tags?

You would check the git history, but I agree its a chore to find what commit triggered what version (since you need to go through the CI logs). For me this is not something I would optimize for, we don't want users to be on the beta releases (only if they don't want to wait for us to do a stable release).

To me, it's just adding (a lot of) noise, for a very low value, but that does not mean I don't see any value - so if you guys want to keep the git tags there, let's keep them (and we can remove later if you change your mind, heh). I would skip the GH releases, that really feels too much - they would always contain a single item only.

Clarification, it's not every commit, the chore/docs commits are ignored.

I don't see a problem with that either, we would ignore those commits in the changelog too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm fine with removing the beta tags - every simplification of that workflow counts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can I also not upload the release artifacts? (see the argument above)

Copy link
Member

Choose a reason for hiding this comment

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

You mean completely, including for stable releases? I guess its fine too, I also don't think people use them, and we can revert this if we hear from someone who would like to do so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding the beta tags - I'm ok with both keeping/removing them, so do as you prefer.

Comment on lines +20 to +21
- auto
- custom
Copy link
Member

Choose a reason for hiding this comment

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

i would like to be able to trigger major/minor/patch specifically (just like we have it in the js crawlee) instead of relying on the git messages. some feature commits are not really worth a feature release (but you want them in the changelog and its just wrong to call them fixes just because of this)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thing is, I'm not aware of a way of doing this with git-cliff specifically. I know this is a weak excuse, but I really liked the idea of letting git-cliff bump our versions without having to search for yet another utility.

Can we open a new issue for this? In the meantime, we can always use the custom mode.

Also, kinda off topic, but some people (such as @netmilk IIRC) would tell you that attaching emotional value to version numbers is wrong and you should just follow the semantic versioning spec like a robot. Every side of the argument has its points IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...it looks like poetry version could also work for us.

Copy link
Member

Choose a reason for hiding this comment

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

How is this about git cliff? You use that to generate changelog between two git tags or commit hashes I'd guess, or its not working that way? The option I am after would only let you decide the version bump manually instead of relying on the git messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how - it has a flag that outputs the new semver based on commits since the last tag. I used it because it was handy. The workflow_dispatch can also accept a custom version number, which will be used directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @B4nan on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we're in any kind of disagreement. You can already set a custom version, can't that be used to achieve the same thing you could do with a patch/minor/major toggle?

The points about marketing and fixing previous mistakes are both valid. While we're at the topic, can we not fix incorrect commit types by push-forcing to master? I'm aware there are tradeoffs, it's an honest question about which ones we picked.

Copy link
Member

@B4nan B4nan Jul 24, 2024

Choose a reason for hiding this comment

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

You can already set a custom version, can't that be used to achieve the same thing you could do with a patch/minor/major toggle?

Yes, it can be used for the same, but is less ergonomic. It means you need to check the version you are on, instead of just releasing the next bump based on the type. You add another possibility for the user to screw it up. I want something in between, not full auto, but not full custom. We are using this for quite some time in crawlee, and it feels really nice, so I would like to see it the same way - especially because we want to port this setup to other repositories in the long run.

While we're at the topic, can we not fix incorrect commit types by push-forcing to master?

Force push to master is a big no-no, especially in open-source repositories. This can piss all the other maintainers collaborators off easily, seen that many times in nette community, as the author liked to do this a lot. You break their local versions since you change commit hashes.

There are exceptions to everything, but I can justify force pushing only for the very last commit done by yourself, within very short time window after they were pushed (lets say minutes).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it can be used for the same, but is less ergonomic. It means you need to check the version you are on, instead of just releasing the next bump based on the type. You add another possibility for the user to screw it up. I want something in between, not full auto, but not full custom. We are using this for quite some time in crawlee, and it feels really nice, so I would like to see it the same way - especially because we want to port this setup to other repositories in the long run.

Sure, I'm in favor of that, but I'd prefer to do that after this PR is merged and tested. Autogenerated changelogs and triggering releases via workflow_dispatch are enough of a milestone IMO.

Force push to master is a big no-no, especially in open-source repositories. This can piss all the other maintainers collaborators off easily, seen that many times in nette community, as the author liked to do this a lot. You break their local versions since you change commit hashes.

Haha, I remember that. I agree that the negative impact on contributors is not worth it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I am fine merging this without it, that's fair. But I would like to see it implemented before next release.

@@ -36,20 +36,109 @@ jobs:
name: Check whether to release
if: |
github.event_name == 'workflow_dispatch' ||
github.event_name == 'release' ||
(
github.event_name == 'push' &&
!startsWith(github.event.head_commit.message, 'docs') &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
!startsWith(github.event.head_commit.message, 'docs') &&
!startsWith(github.event.head_commit.message, 'doc') &&

Copy link
Member

Choose a reason for hiding this comment

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

the commit prefix is called docs: not doc:

https://www.conventionalcommits.org/en/v1.0.0/#summary

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I based this on what we have in cliff.toml, in that case, it should be updated there from doc to docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suspect that git-cliff only puts ^doc in the default configuration to catch both doc: and docs: so that they please everyone 😄

The newly added PR title check should prevent any errors anyway, but I can update cliff.toml too.

vdusek

This comment was marked as resolved.

@janbuchar
Copy link
Collaborator Author

It'd be great to have Check PR title workflow here as well.

Sure, added.

Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

Also please remove check-changelog-entry from .pre-commit-config.yaml.

.github/workflows/run_code_checks.yaml Show resolved Hide resolved
.github/workflows/check_pr_title.yaml Outdated Show resolved Hide resolved
Comment on lines +20 to +21
- auto
- custom
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @B4nan on this.

@janbuchar janbuchar requested a review from vdusek July 24, 2024 11:57
Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

Let's see how it's gonna work, otherwise good job!

@janbuchar janbuchar mentioned this pull request Jul 24, 2024
7 tasks
@janbuchar janbuchar merged commit 9a76ac4 into master Jul 26, 2024
12 of 20 checks passed
@janbuchar janbuchar deleted the autogenerate-changelog branch July 26, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate changelog from the commit messages
3 participants