PLIP: Upgrade warnings in changelog files

[thet]

Proposer : @thet

Seconder : @hvelarde

Abstract

Backwards incompatible changes or changes where users have to adapt customizations should be cleary documented. We already have the practice to document changes in a CHANGES.rst file. Changes which break backwards compatibility are normally also documented as part of a changelog entry. But it's too easy to oversee it. I propose to add a summary of backwards incompatible changes as a summary directly in the changelog file.

Proposal & Implementation

A summary of all changes, which could possibly break customizations or are backwards incompatible should be written on top of each release section in a .. warning:: directive, like so:

1.1 (unreleased)
----------------

.. warning:: Upgrade Warning!
    If you customized the ``batchnavigation.pt`` file or use specific CSS styles for it, you might have to update them to reflect the new HTML structure.

New:

- Refactor batchnavigation HTML layout to use nav/ul/li elements instead of div/span.
  [davilima6]

- Switches deprecated ``listingBar`` CSS class to ``pagination``.
  [davilima6]

Fixes:

- *add item here*

Even simple changes like CSS selector changes should be documented like that.

Such a practice gives more structure in documenting critical changes and raises the visibility of these.

Bigger changes should clearly documented in the Plone Upgrade Guide on docs.plone.org. But having such a Framework Team approved practice should helps us to better track upgrade/migration critical changes and decide to add them to the Plone Upgrade Guide before a Plone release.

Deliverables

• Add documentation about this to https://github.com/plone/documentation/blob/5.0/develop/styleguide/documentation.rst#tracking-changes
• Ask @mauritsvanrees to include it in zest.releaser

  Risks

• People can still forget to document properly

[mauritsvanrees]

This is not something for zest.releaser, but can be added in plone.releaser. This uses zest.releaser entry points. See https://github.com/plone/plone.releaser

But what would need to be added? A question like 'There is no upgrade warning yet, are you sure that is correct?' Then the burden falls on the one doing the releases to somehow check if a warning is needed. The only thing he can do is check the CHANGES.rst. It is easy to miss that such a warning is needed: if I have to minutely start checking commits before doing a release, this is going to be tedious.

Otherwise: +1 for including this. Clarity is good.

[hvelarde]

probably a simpler way to accomplish this is just adding such notes on the entries, for instance:

1.1 (unreleased)
----------------

New:

- Refactor batchnavigation HTML layout to use nav/ul/li elements instead of div/span.
  If you customized the ``batchnavigation.pt`` file or use specific CSS styles for it, you might have to update them to reflect the new HTML structure.
  [davilima6]

- Switches deprecated ``listingBar`` CSS class to ``pagination``.
  [davilima6]

Fixes:

- *add item here*

[thet]

@hvelarde that's what we should already do. But IMO, that's not visible enough and too easy to miss. @mauritsvanrees Something like this can be added to plone.releaser:

.. note:: Upgrade Warning!
    *add item here*

When making releases and nothing is there, the message can be deleted as a whole. I wouldn't expect the release manager to fill that in, that should be done in the process of making the change/pull request.

[mauritsvanrees]

Yes, that would be easy to add here: https://github.com/plone/plone.releaser/blob/ca9a5bbeac7b54d4a145e4a3434ec8374b756aa5/plone/releaser/release.py#L35

[mauritsvanrees]

BTW, with .. note:: you get this:

And with .. warning:: you get this:

So warning gets coloured. I would be in favour of that.

[ebrehault]

+1 for the idea

But regarding implementation, I am not sure: if plone.releaser adds automatically a block like "Warning: add item here", it means we have to manually remove it if there is not applicable warning in our current release. For one publication, that's ok, but when @esteele releases all the packages for a Plone release, it might be very annoying.

[mauritsvanrees]

Well, releasing is handled by a few more people, including me, in the @plone/release-team. And already we must/should check whether there is a Fixes or New heading that is empty. Doing the same for this new warning is okay for me.

Probably we can automate that cleanup in plone.releaser, if we are careful.

O, just in case someone thinks this: people outside the Plone Release Team can still release as well, if they have PyPI rights. It's not that we want to do it all ourselves. But we feel a bit more responsible for doing a couple of releases every now and then. :-)

[ebrehault]

Well, releasing is handled by a few more people, including me, in the @plone/release-team.

sorry @mauritsvanrees, I didn't mean to be rude, I know the release team is doing a great job :) I was just giving an example with @esteele.

people outside the Plone Release Team can still release as well

Yes, I was aware of that. And about that, I was thinking we had to use zest.releaser (that's what the doc says http://docs.plone.org/develop/coredev/docs/release.html ). Are we supposed to use plone.releaser instead?

[mauritsvanrees]

@ebrehault Don't worry, I did not think your comment was rude. I only mentioned the release team to indicate that this plip is not putting a new burden on one single person, but that this burden is distributed over more people, so it automatically is less of a burden.

plone.releaser uses some hooks in zest.releaser to do a few extra checks. If you use plain zest.releaser, you are mostly fine, but plone.releaser for example adds the New/Fixes headers during the postrelease. Run bin/buildout -c release.cfg in coredev 5.0 and you have a bin/fullrelease that includes the plone.releaser code. plone.releaser used to be called esteele.manager, which is still mentioned on the page you link to. This document definitely needs an update. I'll see what I can do. :-)

[gforcada]

Given a formal definition of what defines a deprecation warning, maybe this could be even automated (i.e. if an interface class is removed from interfaces.py, python packages/modules moved around, etc etc) so then a tool could be feed with the last tag and the current tip and compute the diff.

Do we have a definition of what constitutes a breakage that will need migration?

[thet]

Can we agree to accept this? I think this does not need to go through the formal FWT process, or does it? @plone/framework-team @plone/release-team @plone/testing-team @hvelarde

[jensens]

I think its a very good idea to mark incompatibilities.

According to Semantic Versioning this implies also a new Major Version and is relevant for releases.

But following the current schema of sections is more consistent. With a red warning things get confusing over time.

I.e. like so:

2.0 (unreleased)
----------------

Incompatibilities:

- If you customized the ``batchnavigation.pt`` file or use specific CSS styles for it, you might have to update them to reflect the new HTML structure.

New:

- Refactor batchnavigation HTML layout to use nav/ul/li elements instead of div/span.
  [davilima6]

- Switches deprecated ``listingBar`` CSS class to ``pagination``.
  [davilima6]

Fixes:

- *add item here*

1.2 (unreleased)
----------------

[hvelarde]

sounds fair

[thet]

yes, +1

[ebrehault]

+1 and I agree the PLIP process is not needed in that case

[mauritsvanrees]

+1 So, do we add the new Incompatibilities header to all packages after a release? Easy to add in plone.releaser, I can do that. Or do we think incompatibilities do not happen very often, so we only add this header when really needed? Probably best to just add it always (and remove before a release if empty) so developers will be more inclined to add a line there.

[jensens]

I would add always and remove if not needed, otherwise people forget to write a note about incompatibilities.

[ebrehault]

+1 for adding always

[mauritsvanrees]

Done. And for reference, this is how it looks on a test package in case you do not cleanup the empty headers before the release: https://pypi.python.org/pypi/mauritstestpackage/1.18

[jensens]

:+1:

[thet]

@mauritsvanrees thank you!

[esteele]

Is there a sane/safe way to automatically clear those empty sections out when releasing?

[mauritsvanrees]

Is there a sane/safe way to automatically clear those empty sections out when releasing?

I wondered about that too. Especially about the 'safe' way. There you go: https://github.com/plone/plone.releaser/commit/7c3aff6bf959345d8138bbdc1ea0a70ae8c6f0ae

This works best when the changelog file is called CHANGES.rst, as then we can do our stuff before zest.releaser complains about the 'add item here' text. See comments in the cleanup_changelog function.

What is not covered here, is that theoretically someone may have left the 'add item here' in place and added a changelog entry after it. Then our logic removes the header and the 'add item here', leaving the changelog entry either without header or under the wrong header. That should be rare.

[tisto]

A big +1! I think this will improve the overall release process a lot. I think the section titles could be a bit more precise to avoid confusion in my opinion. We should get that right from the beginning, since it will be a lot of work to change those headlines later. Here are my thoughts:

"Incompatibilities" does not make it clear enough that those are is actually a breaking changes. I would prefer to use "Breaking Changes".

"New" is not very precise either. Could be new change, new feature, new breaking change...I think "Features" or "New Features" would be better.

"Fixes" is fine, maybe "Bug Fixes" is a bit more precise.

BREAKING.FEATURE.BUGFIX is a lot easier to remember than INCOMBATIBILITIES.NEW.FIXES. And it aligns better with what other projects are doing (as far as I can tell).

[hvelarde]

@tisto you are the man!

[tisto]

@mauritsvanrees @esteele @thet @jensens @gforcada @ebrehault what do you think about my proposal? I would like to start using this but I don't want to add confusion by using a different style...

[gforcada]

@tisto as long as (zest|plone).releaser adds them we can switch easily. Overall your naming looks better indeed.

[thet]

@tisto +1 from me, I also like your naming suggestion better. We should change. IMO no need to fix old headings for already released packages. It has to be changed here: https://github.com/plone/plone.releaser/blob/master/plone/releaser/release.py

[datakurre]

@tisto :+1: sound semver, which is good

[ebrehault]

+1

[mauritsvanrees]

Seems good. It makes it easier to see that a new release should get a major version bump. We might even be able to test for this in plone.releaser, but I'm not promising anything.

We might want to allow deviating slightly from semantic versioning though. For example plone.app.locales will have 'feature: new translations for language x' in each release, but that does not mean we need a minor version bump each time. Well, might be fine actually... And small new features get into micro CMFPlone releases too...

But in general: semver.org seems good, so it seems useful to change the headings. I can have a go at it.

[tisto]

Thanks @mauritsvanrees! I agree that semantic versioning for Plone is not as easy as it seems. I guess the release team should write down a few guidelines after we gained some experience with it how to handle the "edge cases".

[mauritsvanrees]

I made a pull request for plone.releaser here: https://github.com/plone/plone.releaser/pull/6