Tooling: Introduce pre-commit CHANGELOG verification #13594

aduth · 2019-01-30T14:11:09Z

This pull request seeks to explore adding a pre-commit script which verifies that the contents of the pending commit include a CHANGELOG.md revision if applicable (i.e. within a packages directory, and not considered as an "ignored" change by Lerna).

Testing instructions:

With the branch checked out, verify a few combinations of commit attempts:

A change outside of a package (e.g. root README.md)
- Should succeed
A change to a package-ignored file (e.g. packages/i18n/benchmark/index.js), without CHANGELOG
- Should succeed
A change to a package not-ignored file (e.g. packages/i18n/src/index.js), without CHANGELOG
- Should error
A change to a package not-ignored file (e.g. packages/i18n/src/index.js), with CHANGELOG
- Should succeed

Future tasks:

It would be difficult to integrate this into a build step in its current form, as the script expects to receive the changed file paths as arguments. It should be enhanced to also retrieve a list of changed files from the current branch history.

The current incarnation can serve as a good, less-disruptive initial workflow to allow for some flexibility in the risk of false positives.

nerrad · 2019-01-30T14:24:54Z

So does this mean that every change in a commit must be accompanied by a change in the CHANGELOG.md? What happens when committing a bug fix or something in a watched file as a part of the overall work on the branch where a changelog modification was already made in an earlier commit on the pull?

Assuming most merges of pulls happen via the github ui/ux I wonder if we should instead use the github checks api via a custom github app and it checks there is a changelog entry for the changed code. We probably could base a custom app off of something like this. We could then enforce blocking merges of pulls unless there is a changelog entry.

If there are scenarios where a merge to master happens outside of the github ui then maybe we should investigate also doing a check similar to what you did in here but only on the merge commit to master?

aduth · 2019-01-30T14:30:59Z

So does this mean that every change in a commit must be accompanied by a change in the CHANGELOG.md? What happens when committing a bug fix or something in a watched file as a part of the overall work on the branch where a changelog modification was already made elsewhere?

The problem is that Lerna is dumb, and can't differentiate this point. It sees a touched file, it's going to prompt for publishing a new version. It also can't be on the person doing the publish to follow back the un-logged change to its source.

Presumably, if the file needed to be touched in a package, the change ought to be able to be described as standing on its own legs, as the modules themselves are meant to be standalone, not purely in service of some other thing.

Assuming most merges of pulls happen via the github ui/ux I wonder if we should instead use the github checks api via a custom github app and it checks there is a changelog entry for the changed code. We probably could base a custom app off of something like this.

I started down this path as well, even looking forward to things like actions to make these sorts of workflow automations easier / more integrated to GitHub.

The problem I faced is: I really don't want to go through the trouble of begging for an app to be approved for use in the WordPress organization. Or at least, it seemed there were viable alternatives by starting this as either just a pre-commit check, or as a step in the existing Travis build.

gziolo

I will take a closer look tomorrow but I think this is exactly what we would need for start.

gziolo · 2019-01-30T14:43:54Z

bin/check-changelog.js

+ *
+ * @type {Set}
+ */
+const verified = new Set;


Is it okey to omit parentheses?

Is it okey to omit parentheses?

Yes:

new Foo is equivalent to new Foo(), i.e. if no argument list is specified, Foo is called without arguments.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new#Description

Though some people might argue it's better to just be consistent:

https://eslint.org/docs/rules/new-parens

Interesting. I'm fine to leave it as is if JS is happy :)

gziolo · 2019-01-30T14:46:49Z

bin/check-changelog.js

+ * Example (Error):
+ *
+ * ```
+ * node bin/check-changelog.js /Gutenberg/packages/i18n/src/index.js


Is this comment correct? I would expect something containing Missing CHANGELOG.md entry for...

Is this comment correct? I would expect something containing Missing CHANGELOG.md entry for...

It's an example of a failing call, so yes, it would log the message.

gziolo · 2019-01-31T13:07:42Z

package.json

@@ -197,6 +199,9 @@
 		"*.js": [
 			"wp-scripts lint-js"
 		],
+		"packages/*/**": [


I need to verify my assumption, but it might not work as intended for PR that has multiple commits. Use case I'm about to test is as follows:

add CHANGELOG.md changes upfront and commit

add code changes with another commit

Yes, this is what actually happened:

⚠️ Missing CHANGELOG.md entry for compose package edits.
More information: https://github.com/WordPress/gutenberg/blob/master/packages/README.md#maintaining-changelogs

It works as expected based on the implementation but we need to find a way to improve it :)

gziolo

See my comment: https://github.com/WordPress/gutenberg/pull/13594/files#r252659482

It works fine for a single commit but fails when CHANGELOG.md was already updated in one of the previous commits and isn't included in the commit with code changes.

aduth · 2019-01-31T13:26:56Z

Yes, I can see that being an issue. I'd justified it in my head as the sort of thing where the developer ought to always include the CHANGELOG with the change itself. It ties a bit into the noted "future goal" of allowing the file arguments to be optional and have the tool refer to the branch history instead if omitted.

nerrad · 2019-01-31T14:12:38Z

Yes, I can see that being an issue. I'd justified it in my head as the sort of thing where the developer ought to always include the CHANGELOG with the change itself. It ties a bit into the noted "future goal" of allowing the file arguments to be optional and have the tool refer to the branch history instead if omitted.

This was kind of what I was getting at in my earlier comment though. I think practically, while the initial change a developer commits could include the CHANGELOG entry, pulls often have fixes needed as a result of a review and this creates extra friction for those subsequent commits.

aduth · 2019-01-31T15:44:36Z

This doesn't stop someone from bundling many changes in the same branch, or in the same commit, it just requires that the CHANGELOG for each touched package would be updated as part of the commit, which is the point of the change we're trying to make.

@gziolo's example of preemptively updating a CHANGELOG in a separate, earlier commit is a very niche example; it's not something I'd expect anyone to be doing.

gziolo · 2019-01-31T17:39:35Z

@gziolo's example of preemptively updating a CHANGELOG in a separate, earlier commit is a very niche example; it's not something I'd expect anyone to be doing.

It's extreme but very close to how I patch PRs when I spot mistakes after self-review on GitHub for newly pushed PR.

aduth · 2019-01-31T19:04:02Z

It's extreme but very close to how I patch PRs when I spot mistakes after self-review on GitHub for newly pushed PR.

Maybe I'm misreading, but is that not a commit which takes place after the original change, i.e. the exact thing this pull request would prevent from being allowed to happen?

The problem workflow I see is if someone (a) edited a changelog, (b) committed it, and (c) made the change corresponding the changelog note in a subsequent separate commit.

gziolo · 2019-02-04T12:29:18Z

The problem workflow I see is if someone (a) edited a changelog, (b) committed it, and (c) made the change corresponding the changelog note in a subsequent separate commit.

I think it's every workflow for the existing PR where you add a new commit which contains only code changes but no update to the changelog file.

aduth · 2019-02-04T13:21:13Z

The problem workflow I see is if someone (a) edited a changelog, (b) committed it, and (c) made the change corresponding the changelog note in a subsequent separate commit.

I think it's every workflow for the existing PR where you add a new commit which contains only code changes but no update to the changelog file.

That is what this validation will catch and prevent from happening. The developer can then just add the CHANGELOG entry.

What I meant by "problem" in my previous commit is one which leaves the developer in an awkward unresolvable (at least not without a few extra steps) scenario.

aduth · 2019-02-07T17:40:49Z

Per previous comments and as discussed in this week's #core-js meeting, I've made improvements in a9514c7 to consider whether a previous commit would satisfy the requirement of a CHANGELOG revision having been made for an affected file.

The changes here are also quite nice in letting the script be run directly, without arguments, as a verification of the current branch. In other words, this could be used without any additional changes as part of a Travis task for node bin/check-changelog, but I've opted to not add that quite yet, since I think the precommit alone could prove sufficient at least for the time being. I'm open to reconsidering.

nerrad

I like these changes 👍

gziolo · 2019-02-08T10:33:31Z

bin/check-changelog.js

+			`\u00A0\u00A0\u00A0More information: https://github.com/WordPress/gutenberg/blob/master/packages/README.md#maintaining-changelogs\n`
+		);
+
+		process.exit( 1 );


It will exit as soon as it will find the first violation, there might be more than one if several packages are updated. I don't consider it as a blocker but sharing here so we could discuss whether we should make iterate further.

It will exit as soon as it will find the first violation, there might be more than one if several packages are updated. I don't consider it as a blocker but sharing here so we could discuss whether we should make iterate further.

I can probably improve this without too much trouble 👍

gziolo · 2019-02-08T10:42:49Z

bin/check-changelog.js

+ *
+ * @type {string}
+ */
+const base = exec( 'git merge-base master HEAD' );


This part is tricky and might cause a lot of confusion because we reference the local copy of master branch. I run into this issue locally where my master branch is out of date if you compare with the upstream. Therefore when I run this script I see missing CHANGELOG.md files in folders that weren't updated in the branch where I tried to add a new commit. In fact, I wanted to add a new commit directly to this branch to verify how it would work with some flows. In my case git merge-base origin/master HEAD solves the issue, but that might differ for forks and can be also changed to something else.

I'm also wondering if this isn't a temporary issue caused by the fact that we don't update changelog files in a way this should happen. My assumption is that it wouldn't be an issue when this change lands and folks update their master to the latest version.

As a way to avoid so much confusion maybe we should include the note prompting to update master to the latest version and try again?

On the up to date master branch it works like a charm ✨

This part is tricky and might cause a lot of confusion because we reference the local copy of master branch. I run into this issue locally where my master branch is out of date if you compare with the upstream.

Argh, I'd hoped to have found some reliable way here. The other trouble too with dealing with branches is that Travis does some funny things with branches, which makes using the git command not as reliable (reference).

There's seemingly many ways to check for differences; I want to dig to see if there could be another option here, before resorting to the forced update prompt.

Jest supports something similar with the following flags:

--changedSince - "Runs tests related to the changes since the provided branch. If the current branch has diverged from the given branch, then only changes made locally will be tested."

--onlyChanged - "Attempts to identify which tests to run based on which files have changed in the current repository."

It might be a nice way to compare the approach they took.

youknowriad · 2020-03-18T12:16:54Z

It seems like we very rarely update the package changelogs and it's understandable since it's not enforced. I'd think that we should make a decision. Try to land this PR or abandon these package changelogs entirely. Maybe something for the next #core-js chat

gziolo · 2020-03-18T12:57:20Z

gutenberg/bin/commander.js

Lines 960 to 967 in 548e600

    
           /** 
        
            * Update CHANGELOG files with the new version number for those packages that 
        
            * contain new entries. 
        
            * 
        
            * @param {string} minimumVersionBump Minimum version bump for the packages. 
        
            * @param {string} abortMessage       Abort Message. 
        
            */ 
        
           async function updatePackageChangelogs( minimumVersionBump, abortMessage ) {

We use those CHANGELOG entries to automate version bumps for npm packages. It's very important for the tooling that might have breaking changes more often. I admit it's less of an issue for packages that contain components, but it still might be useful in some cases.

aduth · 2020-03-18T12:59:33Z

@youknowriad I'm keen to revisit this. Especially as the project has matured, the need to effectively communicate changes in individual packages has become increasingly a concern of mine.

But one thing which would be prudent is to try to consider just how we expect these to be used. I think there's a lot of important information that can be included in these CHANGELOGs, and it's for this reason we've thought it crucial they be updated. (Aside: Looking back at my original comment, I feel I could have been more clear with these reasons)

On the consuming end, we might want to think more about: (a) if and how they get incorporated into the Gutenberg release notes, (b) how someone like a plugin author can use these notes to know about upcoming or past changes associated with a WordPress release. A CHANGELOG workflow may be familiar for someone consuming packages from NPM, and this is certainly a use-case we want to support via these updates. I think it's also fair to say that at the current moment, it's not necessarily the primary one motivating these changes (save for a few packages like scripts, eslint-plugin, etc).

Or, at least, in addressing the other concern about how this may be viewed as a burden for contributors who aren't currently used to this being part of the workflow, it would be easier to do so if we could can clearly demonstrate the value that they bring.

I have a few more specific tactical remarks I have in mind here. I'll consider those in a separate comment.

aduth · 2022-05-27T22:52:35Z

Clearly not a lot of urgency with this one, so will go ahead and close as stale.

Future work could pick this back up, mindful of the fact that some of the git operations may need to be updated (e.g. references to "master" branch).

aduth added the [Type] Build Tooling Issues or PRs related to build tooling label Jan 30, 2019

gziolo self-requested a review January 30, 2019 14:34

gziolo reviewed Jan 30, 2019

View reviewed changes

gziolo reviewed Jan 31, 2019

View reviewed changes

gziolo requested changes Jan 31, 2019

View reviewed changes

gziolo added the [Type] Project Management Meta-issues related to project management of Gutenberg label Jan 31, 2019

aduth requested review from ajitbohra, nerrad, ntwb and youknowriad as code owners February 7, 2019 17:38

nerrad approved these changes Feb 7, 2019

View reviewed changes

aduth added 2 commits February 7, 2019 14:01

Tooling: Introduce pre-commit CHANGELOG verification

b1d5442

Tooling; Consider previous commits in CHANGELOG validation

5509cc4

aduth force-pushed the try/check-changelog branch from 8f181e8 to 5509cc4 Compare February 7, 2019 19:01

gziolo reviewed Feb 8, 2019

View reviewed changes

gziolo added this to the 5.1 (Gutenberg) milestone Feb 8, 2019

aduth modified the milestones: 5.1 (Gutenberg), 5.2 (Gutenberg) Feb 15, 2019

ntwb mentioned this pull request Feb 19, 2019

Convention for monorepos conventional-commits/conventionalcommits.org#118

Closed

aduth mentioned this pull request Feb 25, 2019

Focal Point Picker #10925

Merged

4 tasks

gziolo modified the milestones: 5.2 (Gutenberg), 5.3 (Gutenberg) Mar 4, 2019

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

youknowriad mentioned this pull request Apr 18, 2019

Incorrect version in CHANGELOG.md for @wordpress/data #15030

Closed

aduth mentioned this pull request May 22, 2019

Framework: Update "Maintaining Changelogs" section to current recommendations #15740

Merged

Base automatically changed from master to trunk March 1, 2021 15:42

aduth closed this May 27, 2022

aduth deleted the try/check-changelog branch May 27, 2022 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tooling: Introduce pre-commit CHANGELOG verification #13594

Tooling: Introduce pre-commit CHANGELOG verification #13594

aduth commented Jan 30, 2019

nerrad commented Jan 30, 2019 •

edited

Loading

aduth commented Jan 30, 2019 •

edited

Loading

gziolo left a comment

gziolo Jan 30, 2019

aduth Jan 30, 2019

gziolo Jan 31, 2019

gziolo Jan 30, 2019

aduth Jan 30, 2019

gziolo Jan 31, 2019

gziolo Jan 31, 2019

gziolo left a comment

aduth commented Jan 31, 2019

nerrad commented Jan 31, 2019

aduth commented Jan 31, 2019

gziolo commented Jan 31, 2019

aduth commented Jan 31, 2019 •

edited

Loading

gziolo commented Feb 4, 2019

aduth commented Feb 4, 2019

aduth commented Feb 7, 2019

nerrad left a comment

gziolo Feb 8, 2019

aduth Feb 8, 2019

gziolo Feb 8, 2019

gziolo Feb 8, 2019

aduth Feb 8, 2019

gziolo Feb 11, 2019

youknowriad commented Mar 18, 2020

gziolo commented Mar 18, 2020

aduth commented Mar 18, 2020

aduth commented May 27, 2022

Tooling: Introduce pre-commit CHANGELOG verification #13594

Tooling: Introduce pre-commit CHANGELOG verification #13594

Conversation

aduth commented Jan 30, 2019

nerrad commented Jan 30, 2019 • edited Loading

aduth commented Jan 30, 2019 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Jan 31, 2019

nerrad commented Jan 31, 2019

aduth commented Jan 31, 2019

gziolo commented Jan 31, 2019

aduth commented Jan 31, 2019 • edited Loading

gziolo commented Feb 4, 2019

aduth commented Feb 4, 2019

aduth commented Feb 7, 2019

nerrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Mar 18, 2020

gziolo commented Mar 18, 2020

aduth commented Mar 18, 2020

aduth commented May 27, 2022

nerrad commented Jan 30, 2019 •

edited

Loading

aduth commented Jan 30, 2019 •

edited

Loading

aduth commented Jan 31, 2019 •

edited

Loading