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 changelog management #1987

Closed
ewels opened this issue Aug 16, 2023 · 8 comments
Closed

Better changelog management #1987

ewels opened this issue Aug 16, 2023 · 8 comments
Labels
core: infrastructure Something to do with the MultiQC infra documentation

Comments

@ewels
Copy link
Member

ewels commented Aug 16, 2023

A classic problem with managing pull-requests in this project is merge conflicts arising from changelog entries.

This isn't a new problem, and there are tools that have been written to address this. For example:

It'd be good to have a play with these and see if we like them / if they're worth the effort of setting up and using.

I suspect that many contributors won't want to use these tools locally. So ideally it would be paried with a GitHub action where a changelog entry can be added to a PR via a special comment on the PR or something. That'd be nice.

@vladsavelyev
Copy link
Member

The way I see it is that when we are preparing a release, we run a script that takes descriptions of all merged pull requests, and simply populates a change log.

So care should be taken into writing the pull request titles. And they should be prefixed with labels like New module: Samtools and Samtools: fix parsing of input files (or [New module] Samtools/[Samtools]: fix parsing of input files)

@ewels
Copy link
Member Author

ewels commented Aug 31, 2023

Let's use standard semantic commit message structure: https://www.conventionalcommits.org/

Can then use stuff like https://github.com/marketplace/actions/semantic-pull-request

Need to think about our own categories, for example to distinguish between adding a new module and adding a new feature to an existing module.

@vladsavelyev
Copy link
Member

I tried https://www.conventionalcommits.org for a few PRs, and it doesn't look very aesthetic to me:

Screenshot 2023-09-02 at 23 46 27

I think their model implies that there is a limited number of possible "scopes" that you can list in the CI config: like ui, api, backend, etc. For us, we have modules that are added all the time, and using a module name as a scope in parentheses looks ugly'ish. Also, constructions like feat(core) look a bit unnecessary geeky and could be confusing to contributors who only start programming.

Also, it's not clear how to name a "new module" PR in their model - and that's the most frequent type of PR in MultiQC.

Maybe we could stick to the following - pretty much the existing but not strictly followed - guideline:

  • New module: Bakta (must start with the words "New module: " or similar, will be listed under the corresponding section in the change log).
  • Kraken: Support new format (if a PR name starts with a name of an existing module, it will go under "Module updates" in the change log).
  • Everything else will go under "MultiQC updates" in the changelog.

Can potentially split the latter it into subsections:

  • Features
  • Documentation
  • Bug fixes
  • Refactoring/optimization

By allowing prefixing commit names with New feature:, Fix:, etc.

@ewels
Copy link
Member Author

ewels commented Sep 3, 2023

Agree - I also don't want to put people off with unfamiliar terminology. Especially seeing your test I agree that it's not worth following existing conventions in our case. Let's keep it natural.

I guess we will write instructions for this, but I also suspect that it's something that we will be updating ourselves most of the time 😅 A CI test could help to check, the only downside is that it might be difficult / impossible to get the CI test to "refresh" after the title is updated?

We also need a category for "don't include in the changelog at all", for small things.

@ewels
Copy link
Member Author

ewels commented Sep 3, 2023

CI tests could even be clever, doing stuff like checking module names against the list of installed modules.

We also need to think about how to get additional info into the changelog. The PR title is good for the first line / grouping. But new modules usually have a tool description and link, for example.

@ewels
Copy link
Member Author

ewels commented Sep 3, 2023

One last thing: this will mean that the changelog doesn't get updated until the release is about to go out. Bit of a shame that we lose the running changelog of the current dev version.

I wonder if we could update the changelog automatically on merge? The PR merge conflicts should still be avoided I think.

@vladsavelyev
Copy link
Member

I wonder if we could update the changelog automatically on merge?

Yeah, I think that can be done!

We also need to think about how to get additional info into the changelog. The PR title is good for the first line / grouping. But new modules usually have a tool description and link, for example.

I think that could be specified once, e.g. in the code in initialisation:

        super(MultiqcModule, self).__init__(
            name="Bakta",
            href="https://github.com/oschwengers/bakta",
            info="Rapid and standardized annotation of bacterial genomes, MAGs & plasmids.",

And used by CI to generate the md file (as we discussed) and for the change log.

@ewels
Copy link
Member Author

ewels commented Sep 3, 2023

True 👍🏻 🤔

I think that there will still be some cases where additional custom changelog stuff will be nice. But I guess we can always still just do that via a PR manually as before. Nothing to stop tweaks like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core: infrastructure Something to do with the MultiQC infra documentation
Projects
None yet
Development

No branches or pull requests

2 participants