search

twisted / towncrier

Manage the release notes for your project.
https://towncrier.readthedocs.io
MIT License
777 stars 121 forks source link

Support explicitly selected 'change' and 'release' checks #309

Open altendky opened 3 years ago

altendky commented 3 years ago

This is maybe a piece of, or duplicate of, or related to, or... issues #152 and #246 and also PRs #174, #296, and #337. This issue is being created as a clear place to discuss addition of an explicit control between two types of checks and what those checks would do. The other issues either describe a problem and a general request. The PRs (one built from the other) implement weak implicit detection and checks.

Given that the subcommand interface is only available presently in 19.9.0rc1 I think it is ok to change that without deprecation etc (I don't know if the option-based CLI was even deprecated). @energizah suggested that we consider names relating to what the checks do which seems at the very least worth considering. I will be referring to them as the change check and the release check for now without implying any decision against the suggestion. @adiroiban suggested the existing check be documented in the readme first. I think we should know what it does but I'm not sure it needs to go in the readme if we plan to change it or it's surroundings before the next release? We should be sure to remember the -m towncrier interface as well while considering the design of the explicit check-type selection (I say this because I had kind of forgotten about it).

Checks:

This is just an initial quick list, I started into this a bit later than I ought have.

Just to make sure everyone sees this: @adiroiban, @energizah, @freakboy3742, @tjanez, @vcalvert. Obviously feel free to unsubscribe if you don't want to follow this new issue.

adiroiban commented 3 years ago

Thanks @altendky for putting this together

I am -1 on naming based on what it does (functional name)... the check might do many things.

I am +1 for naming it based on when you need to use the check (semantic name) - on a non-release branch vs on the release branch.

I am also using towncrier --draft as part of my "change" checks to make sure towncrier is configured OK and it will not unexpectedly fail on the release branch. So I would argue that the build process done by --draft should be included in the --check process.

A release candidate, is kind of a public release . It's not a dev /beta / alpha release and is not ok to change the interface.

I would say that adding subcommnads for towncrier is over-engineering and we should keep it simple with arguments :) But I am ok to use subcommands... this is not a big deal :)

altendky commented 3 years ago

Maybe the 'change' check ought to go beyond draft and actually write into 'the file'? Either undoing itself after or copying the file to a temporary location and writing into it. Even if so, draft might be a sensible intermediate step. Or perhaps there should be a 'full-draft' ability that reads the target file and inserts into the proper place but writes to stdout rather than back to the file.

Also, why would this only be relevant to the 'release' check? Shouldn't a 'change' also be ready to be built?

Ok, so if we do have to treat changing the subcommands in an incompatible way as a relevant breaking change, what's the deprecation policy in Towncrier? :]

So it appears that backwards compatibility was retained while by 1) adding subcommands and 2) setting the default subcommand to build and keeping that compatible (at least 'ish, I didn't analyze in any detail). So either keeping the existing towncrier check subcommand and adding towncrier release-check or such would retain compatibility, or having towncrier check change being a new sub-sub-command and being the default ofr check and also adding towncrier check release. Regardless, there are multiple paths forward it seems while retaining compatibility.

So there are already subcommands and they pre-exist me jumping into this. Also, my basic perspective on subcommands is that they let you create exclusive sets of options. Given that we are concluding that the 'change' and 'release' checks are significantly different activities it seems vaguely likely that in so much as they have options they won't all be shared. But sure, we can design out the needs of each and see where it really ends up for now. Interdependent options just aren't fun.

adiroiban commented 3 years ago

Thanks @altendky for the followup and your care for towncrier :)

When I say "pre-release", I mean "change" check.

We can go with subcommands. No problem.

My comment about A release candidate, is kind of a public release was only a general one. Without any comment on the towcrier backward compatibility policy.

I think keeping a deprecation for 1 year is good enough for such a small project... and it;s also a dev tool.

I think that for towncrier we should just go directly and make public releases. If something goes wrong, we just do a bug/patch release.

We have automated tests so I don't see the point of a release candidate.

I view the release candidate as suitable for projects that need a lot of manual testing.

As you mention, I think that we are ok with the towncrier check backward compatibility.

adiroiban commented 2 years ago

So in summary.

I prefer a dedicated towncrier check --release command. But this command would require extra code in your CI script.

So, as a "just works(tm)" solution, I think that using NEWS.rst changes as an indicator for a release branch is good enough..and it makes it very easy to use towncrier check with a CI.

Later we can have towncrier check --strict that disables this heuristic and towncrier check --release to do explicit release time checks.

adiroiban commented 1 year ago

We also have the issue of a "revert" checks.

On a revert PR you should not have any news fragment, as the revert is just removing existing ones.

Maybe the "skip" logic that was implemented in #337 can be change to skip when a newsfragment file is removed from git.

We removed newsfragments on release branches and on revert branches.

webknjaz commented 1 year ago

FWIW, the heuristic I use for my bot, checks for adding things to the changelog while seeing fragments being deleted: https://github.com/sanitizers/chronographer-github-app/commit/981b4e#diff-f5a9ea88bb1aae73bbebd96a3ab3f26fe53cd70aa0fcfa29bc4c316e63b66208R382-R412

adiroiban commented 1 month ago

With #337 merge, what I think that is missing is failing on a release branch if there are still unprocessed news fragment files.

This can be a bit complicated, as you might want to update the NEWS file header as part of a non-release PR ...

So maybe the only way to fix this is to have towncrier check --release

Expected behaviour

Looking at these files:
----
1. /home/adi/chevah/towncrier/src/towncrier/newsfragments/123.misc
2. /home/adi/chevah/towncrier/NEWS.rst
----
Error: News file changes detected while there are still unprocessed newsfragments.

actual result.

Looking at these files:
----
1. /home/adi/chevah/towncrier/src/towncrier/newsfragments/123.misc
2. /home/adi/chevah/towncrier/NEWS.rst
----
Checks SKIPPED: news file changes detected.