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

Tooling: Introduce pre-commit CHANGELOG verification #13594

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@aduth
Copy link
Member

aduth commented Jan 30, 2019

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

This comment has been minimized.

Copy link
Contributor

nerrad commented Jan 30, 2019

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

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 30, 2019

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 gziolo self-requested a review Jan 30, 2019

@gziolo
Copy link
Member

gziolo left a comment

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

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

This comment has been minimized.

@gziolo

gziolo Jan 30, 2019

Member

Is it okey to omit parentheses?

This comment has been minimized.

@aduth

aduth Jan 30, 2019

Author Member

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

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

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

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

This comment has been minimized.

@gziolo

gziolo Jan 30, 2019

Member

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

This comment has been minimized.

@aduth

aduth Jan 30, 2019

Author Member

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.

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

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

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

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

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
Copy link
Member

gziolo left a comment

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

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Contributor

nerrad commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member

gziolo commented Jan 31, 2019

@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

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member

gziolo commented Feb 4, 2019

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

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 4, 2019

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 aduth requested review from ajitbohra , nerrad , ntwb and youknowriad as code owners Feb 7, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 7, 2019

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

nerrad approved these changes Feb 7, 2019

Copy link
Contributor

nerrad left a comment

I like these changes 👍

@aduth aduth force-pushed the try/check-changelog branch from 8f181e8 to 5509cc4 Feb 7, 2019

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

process.exit( 1 );

This comment has been minimized.

@gziolo

gziolo Feb 8, 2019

Member

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.

This comment has been minimized.

@aduth

aduth Feb 8, 2019

Author Member

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 👍

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

This comment has been minimized.

@gziolo

gziolo Feb 8, 2019

Member

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?

This comment has been minimized.

@gziolo

gziolo Feb 8, 2019

Member

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

This comment has been minimized.

@aduth

aduth Feb 8, 2019

Author Member

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.

This comment has been minimized.

@gziolo

gziolo Feb 11, 2019

Member

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.

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

@ntwb ntwb referenced this pull request Feb 19, 2019

Open

Convention for monorepos #118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment