Automatically generate changelog revision text if the changelog was not hand-crafted - do not commit if unchanged #58

weierophinney · 2020-08-25T20:47:33Z

Q	A
Bugfix	yes
BC Break	code, yes; UI and usage, no
New Feature	yes and no
RFC	yes

Description

When rolling out automatic-releases to various repositories, we noticed an issue with the workflow for documentation, mainly that our docs-build action only builds on pushes to the "master" branch, we are trying to eliminate. This has meant manually pushing the current release branch to the "master" branch on the repository whenever there are changes.

@Ocramius suggested that we merge documentation changes just like bugfixes: assign them to milestones, and build on release. The objections raised were:

We would have unchanged packages from one version to the next, as docs are excluded via .gitattributes. We decided this was not really an issue; most people are not updating regularly, and a package that introduces zero code changes is generally welcome as it represents no risk.
We would have empty changelogs and release announcements.

This latter was something I realized we could address in the automatic-releases tooling, as:

We already generate additional release notes via jwage/changelog-generator.
The phly/keep-a-changelog tooling does not require that entries are in a specific format unless you want to use its entry:* commands; the only requirements are the version headings.

As such, this patch refactors how changelogs are generated for release notes, and then committed back to the repository.

In particular:

It introduces Laminas\AutomaticReleases\Changelog\ChangelogReleaseNotes, which is an entity that composes the changelog contents, and optionally a phly/keep-a-changelog ChangelogEntry instance. It has behavior:
- requiresUpdatingChangelogFile() returns true if a ChangelogEntry is composed AND its contents differ from those stored in the ChangelogReleaseNotes instance.
- writeChangelogFile(string $changelogFile) will write the release notes contents to the specified changelog file, per the metadata in the ChangelogEntry instance composed.
- merge(ChangelogReleaseNotes $next): ChangelogReleaseNotes will clone itself and merge $next into it, returning the clone.
I updated CreateReleaseText to have it return a ChangelogReleaseNotes instance.
I updated CreateReleaseTextViaKeepAChangelog to now mimics how the phly/keep-a-changelog library parses a changelog file when parsing string contents representing a full changelog as pulled from the originating branch, in order to create a ChangelogEntry that details where the original is found in the file, and how much space it takes. Additionally, it then sets the date for the version, and checks to see if the contents have changed from the template; if not, it clears them before passing them to the ChangelogReleaseNotes instance it creates.
I updated CreateReleaseThroughChangelog to perform some minor markdown formatting changes to better accommodate being injected in a CHANGELOG.md file; it also now returns a ChangelogReleaseNotes instance (without a ChangelogEntry composed).
I updated CommitReleaseChangelog to accept a ChangelogReleaseNotes as the first argument; implementations now use it to actually write the CHANGELOG.md file prior to committing and pushing it.
I renamed ConcatenateMultipleReleaseTexts to MergeMultipleReleaseNotes to better describe what it now does (it uses the merge() operation of returned ChangelogReleaseNotes instances, returning the final product).
I updated the logic of ReleaseCommand to generate release text first, then commit it.

tl;dr

When the "release" command of automatic-releases now runs, it will:

Always generate release notes using jwage/changelog-generator.
IF a CHANGELOG.md file is present, it then:
- Checks to see if it contains any changes for the target version. If not, it replaces the content with the generated release notes.
- Otherwise, it appends the content with the generated release notes.

The upshot is that documentation maintainers can:

Add documentation patches to the appropriate milestone.
Merge those changes.
Close the milestone.

and get a release issued, which will then trigger builds for documentation.

Final note

We will need to use the ORGANIZATION_ADMIN_TOKEN for the value of the GITHUB_TOKEN on the Release step of the workflow in order to trigger the release workflow event, and thus the documentation build.

- Use a third-level heading for the opening statement, and chnage from "Release ..." to "Release Notes for ..." - Rewrite '----' and '====' style headings to appropriate `#` style headings (consistency) Signed-off-by: Matthew Weier O'Phinney <[email protected]>

This patch accomplishes several goals: - When a changelog entry from a CHANGELOG.md file does not contain any changes, its contents are removed. - The contents from jwage changelog generator either replace those in for the entry in CHANGELOG.md (if it had no changes), or append to it, ALWAYS. These are then COMMITTED to the file. To accomplish this, I needed to do the following: - I created the class `ChangelogReleaseNotes`. It aggregates changelog contents, and, optionally, a phly/keep-a-changelog `ChangelogEntry` instance. It defines three behavior methods in addition to its `contents()` value method: - `requiresUpdatingChangelogFile()`, which returns true if there is a changelog entry instance AND its contents differ from those of the `ChangelogReleaseNotes` instance itself. - `writeChangelogFile(string $changelogFile)` will write the contents aggregated in the instance to the specified changelog file, per the composed `$changelogEntry` instance - `merge(ChangelogReleaseNotes $next): ChangelogReleaseNotes` will create a new instance that merges the current with the one provided. The contents of the one provided are appended to the original using a separator, and if either compose a `ChangelogEntry` instance, it will be used. (If both define one, this is considered undefined behavior.) - I updated the CreateReleaseText interface to return a `ChangelogReleaseNotes` instance. - I updated the `CreateReleaseTextViaKeepAChangelog` to compose a `Clock` instance, and to inline logic for creating a `ChangelogEntry` instance when parsing the `CHANGELOG.md` file for the related version. Once it has, it removes the entry if it is unchanged since initialization, and sets the date for the release before seeding a `ChangelogReleaseNotes` instance to return. - I updated `CreateReleaseTextThroughChangelog` to create and return a `ChangelogReleaseNotes` instance with the text it creates. - I renamed `ConcatenateMultipleReleaseTexts` to `MergeMultipleReleaseNotes`, and have it now using `array_reduce` on its aggregated items to `merge()` each result with the previous, returning the merged `ChangelogReleaseNotes` instance. - I updated the `CommitReleaseChangelog` interface to accept a `ChangelogReleaseNotes` instance as the first argument. It then test to see if it has changes via `requiresUpdatingChangelogFile()`, and, if so, it uses its `writeChangelogFile()` method to write the changes before committing them. - In `ReleaseCommand`, I inverted the release text creation and commit operations, and capture the `ChangelogReleaseNotes` for use with both the commit and the release operations. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Adds information detailing user-facing changes of laminas#58. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Ocramius

One potential bug, some space for refactoring, otherwise requires commenting on tricky logic that parses release notes.

I think this needs only quick adjustments, and then we can release it as 1.3.0

src/Changelog/ChangelogReleaseNotes.php

Ocramius · 2020-08-26T01:06:04Z

src/Application/Command/ReleaseCommand.php

            $milestone,
            $repositoryName,
            $releaseVersion,
            $releaseBranch,
            $repositoryPath
        );

+        ($this->commitChangelog)($changelogReleaseNotes, $repositoryPath, $releaseVersion, $releaseBranch);


I like how this now receives a pre-computed changelog 👍

I thought you might. 😄

This made me happy as well. The "commit, then create" from before worked, and was necessary based on what we were doing, but this makes sooooo much more sense.

src/Changelog/CommitReleaseChangelogViaKeepAChangelog.php

Ocramius · 2020-08-26T01:10:34Z

src/Github/CreateReleaseTextThroughChangelog.php

+        $lines         = explode("\n", trim($changelog));
+        $lineCount     = count($lines);
+        $linesToRemove = [];
+        for ($i = 0; $i < $lineCount; $i += 1) {


This code block is really hard to understand. I suppose most of it is tested well, according to mutation testing results, but it is really making the reader's head spin.

Refactoring can be deferred, since the code is functionally pure, but we need comments about what is being done here.

how about using foreach instead ?

Suggested change

for ($i = 0; $i < $lineCount; $i += 1) {

foreach ($lines as $i => $line) {

We can eliminate usage of $lineCount = count($lines);

src/Github/CreateReleaseTextThroughChangelog.php

src/Github/CreateReleaseTextViaKeepAChangelog.php

Ocramius · 2020-08-26T01:14:52Z

src/Github/MergeMultipleReleaseNotes.php

+        );
+
+        Assert::isInstanceOf($releaseNotes, ChangelogReleaseNotes::class);
+        Assert::notEmpty($releaseNotes->contents());


Is this assertion needed? I think ->contents() already returns a non-empty-string

test/unit/Changelog/ChangelogReleaseNotesTest.php

Ocramius · 2020-08-26T01:16:59Z

test/unit/Github/CreateChangelogTextTest.php

 use PHPUnit\Framework\TestCase;

 final class CreateChangelogTextTest extends TestCase
 {
    public function testGeneratedReleaseText(): void
    {
+        /** @psalm-var GenerateChangelog&MockObject $generateChangelog */


This inline type shouldn't be needed: phpunit mocking utilities already have the correct annotations

This was in part for intelephense code completion.

Doesn't intelephense read something like .phpstorm.meta.php?

samsonasik · 2020-08-26T01:31:57Z

src/Github/CreateReleaseTextThroughChangelog.php

+    private function normalizeChangelogHeadings(string $changelog): string
+    {
+        $lines         = explode("\n", trim($changelog));
+        $lineCount     = count($lines);


can be removed if next for changed to foreach

michalbundyra · 2020-08-26T05:35:43Z

src/Github/CreateReleaseText.php

     */
    public function __invoke(
        Milestone $milestone,
        RepositoryName $repositoryName,
        SemVerVersion $semVerVersion,
        BranchName $sourceBranch,
        string $repositoryDirectory
-    ): string;
+    ): ChangelogReleaseNotes;


BC Break 😱 Technically then this feature should go to v2.0

This package has "type": "application" and is not published to packagist either - we can break internals as much as we want (and feel free to do so!)

Requires making the `writeChangelogFile()` method static. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Details the algorithms for each, and what they are expected to accomplish. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Already guaranteed by `ChangelogReleaseNotes` method definition. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

- Adds `@covers` annotation to `ChangelogReleaseNotesTest` - Removes unnecessary variable annotation in `CreateChangelogTextTest` Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Ocramius

LGTM 🚢

Ocramius · 2020-08-26T15:42:55Z

Ah, can't merge this due to mutation score going down quite a lot. This is most likely due to the complex logic in the changelog text generation/extraction code: we can either test it more carefully (more string examples, effectively), or rewrite bits of it to be more functional-oriented, and removing prodcedural/iteration bits that are more mutation-prone (yes, FP has the advantage of reducing code paths :D )

Requires fully qualified class name, not one derived from import statements. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Updates test cases to ensure all paths of parsers and normalizers are tested. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

weierophinney · 2020-08-26T19:00:47Z

Mutation score fixed, @Ocramius 😄

To simplify setup in our own repos, I'm adding another example workflow file that uses the organization admin token in the release step, to allow triggering a release workflow event (which will be necessary for us to trigger documenation builds). Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Ocramius · 2020-08-26T19:23:16Z

examples/.github/workflows/release-on-milestone-closed-triggering-release-event.yml

@@ -0,0 +1,57 @@
+# https://help.github.com/en/categories/automating-your-workflow-with-github-actions


This example needs a better comment

Ocramius

LGTM - the documentation of the new example can wait a bit

weierophinney added 2 commits August 20, 2020 15:56

weierophinney added Enhancement Review Needed labels Aug 25, 2020

weierophinney added this to the 1.3.0 milestone Aug 25, 2020

docs: updates CHANGELOG.md

4ea82d5

Adds information detailing user-facing changes of laminas#58. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

weierophinney marked this pull request as ready for review August 25, 2020 20:55

weierophinney mentioned this pull request Aug 25, 2020

Build only latest tags laminas/documentation-theme#50

Merged

Ocramius requested changes Aug 26, 2020

View reviewed changes

samsonasik reviewed Aug 26, 2020

View reviewed changes

michalbundyra requested changes Aug 26, 2020

View reviewed changes

weierophinney added 4 commits August 26, 2020 08:28

qa: mark ChangelogReleaseNotes as immutable

9b2a547

Requires making the `writeChangelogFile()` method static. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

qa: document logic of parsing and normalization of release notes

fca80b8

Details the algorithms for each, and what they are expected to accomplish. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

qa: remove unnecessary assertion

e5b74ae

Already guaranteed by `ChangelogReleaseNotes` method definition. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

qa: testing improvements

6ef4255

- Adds `@covers` annotation to `ChangelogReleaseNotesTest` - Removes unnecessary variable annotation in `CreateChangelogTextTest` Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Ocramius approved these changes Aug 26, 2020

View reviewed changes

weierophinney added 2 commits August 26, 2020 13:46

qa: fix @Covers annotation

7dfb68c

Requires fully qualified class name, not one derived from import statements. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

qa: increase test coverage

99877aa

Updates test cases to ensure all paths of parsers and normalizers are tested. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Ocramius reviewed Aug 26, 2020

View reviewed changes

Ocramius approved these changes Aug 26, 2020

View reviewed changes

Ocramius merged commit a841f63 into laminas:1.3.x Aug 26, 2020

Ocramius changed the title ~~Feature/changelog revisions~~ Automatically generate changelog revision text if the changelog was not hand-crafted - do not commit if unchanged Aug 26, 2020

Ocramius self-assigned this Aug 26, 2020

weierophinney deleted the feature/changelog-revisions branch August 26, 2020 19:28

	for ($i = 0; $i < $lineCount; $i += 1) {
	foreach ($lines as $i => $line) {

		@@ -0,0 +1,57 @@
		# https://help.github.com/en/categories/automating-your-workflow-with-github-actions

Uh oh!

Automatically generate changelog revision text if the changelog was not hand-crafted - do not commit if unchanged #58

Automatically generate changelog revision text if the changelog was not hand-crafted - do not commit if unchanged #58

Uh oh!

Conversation

weierophinney commented Aug 25, 2020

Description

tl;dr

Final note

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Aug 26, 2020

Uh oh!

weierophinney commented Aug 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!