[Tooling] CHANGELOG and Version Bump PreCommit Hook

[h5law]

Description

This PR contains the pre-commit logic to check and validate whether the relevant CHANGELOG.md files have (1) been included in the commit and (2) contain up-to-date versioning. This is in response to Issue #93.

Git Hook:
The client-side logic is contained in the bash script .githooks/pre-commit for it to actually perform this verification check prior to a commit it must be copied into .git/hooks and be executable.

cp .githooks/pre-commit .git/hooks
chmod +x .git/hooks/pre-commit

As it is a git hook, it can be disabled locally by running git commit with the --no-verify or -n flag:

git commit -n ...

The script will detect if a CHANGELOG.md is present and if it is it will check whether (1) the latest version in the changelog is greater than the one before it and (2) that the date alongside the latest version is the current date. If any of these checks fail (no changelog, invalid version, not current date) then the script will block the commit.

To verify the changelog's version is correct it converts the string [0.0.0.8] to 0008 and then strips the leading zeros and compares the number against the next version found if it is less than or equal to the one previous it sees this as an error. This is done by extracting the versions and dates from the CHANGELOG.md file using sed. It then compares the date of the newest version against today's date and if they aren't equal and it sees this as an error.

Currently, the script uses the directories .githooks .github docs bin as ignored directories and does not check for a CHANGELOG.md file in these subdirs.

Dependencies:
This script requires bash to be installed and requires sed, head and date commands - these should be present on all Linux/OSX/BSD systems. The script uses sed Basic Regular Expressions that are portable across GNU sed and FreeBSD sed (used by OSX as well) and also uses date formatting that is consistent across Linux/OSX/BSD versions of the command. Currently, there is no support for Windows when run locally.

GitHub Workflow:
The GitHub workflow will run the pre-commit script however it will populate the edited files by instead passing them as arguments to the script. This cannot be disabled and will run on all PRs when they are opened, reopened and synchronised. The jobs are configured to run on ubuntu-latest so there are no issues with dependencies as all the required binaries are present on ubuntu installs.

The files in question are found in:

.github
└── [ 112] workflows
      └── [ 1KB] changelog-verify.yml
.githooks
└── [ 4KB] pre-commit
docs
└── [ 60] development
      └── [ 6KB] README.md

Issue

Fixes #93

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

• Add pre-commit git hook to validate changelogs presence in commits & correct versioning
• Add GitHub workflow to run the pre-commit logic on all PRs [opened, reopened, synchronised]
• Add a guide to activate pre-commit hook to docs/development/README.md
• Updated utility/doc/CHANGELOG.md

Testing

Videos:
changelog-verify-test-video.webm
github-workflow-test-video1.webm
github-workflow-test-video2.webm

Required Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have tested my changes using the available tooling --> I have tested locally, and using GitHub PRs for the workflow
• I have updated the corresponding CHANGELOG

If Applicable Checklist

• I have updated the corresponding README(s); local and/or global
• I have added tests that prove my fix is effective or that my feature works
• I have added, or updated, mermaid.js diagrams in the corresponding README(s)
• I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

[Olshansk]

@h5law First off, thank you! Love the clear explanation, documentation, videos, quality of work and how just got it frikkin done 💯


_My only request (if you plan on contributing in the future) is just to leave a comment on the issue (#93) next time to check with the team that it’s still relevant or to make sure you have a lock on the task._


My bash-foo is not so strong, so I don't have any real feedback unfortunately other than some nits 🙇. It's pretty easy to follow, the comments are helpful and clear enough to let us make changes if/when need be.

Left a couple minor comments and also shared with the team internally!

[h5law]

I have addressed the comments you gave @Olshansk you didn't mention it so I think it will be fine but if you want me to stop checking for changes in the root directory (no real code there + no changelog) and any others let me know I can quickly do this if needed.

[Olshansk]

A couple more comments, a question and then good to go

[h5law]

@Olshansk I have addressed the issues you brought up in the documentation file and added a third job to run after the validation step in the workflow. If this job sees that the validation step failed, it will automatically add the 'do not merge' label, which should trigger the pre-existing do-not-merge workflow to prevent any merging until the label is removed. I don't think automatically removing the label if the test passes would be a good idea as it may be applied for some other reason besides a CHANGELOG so I left it at simply applying the label.

[Olshansk]

it will automatically add the 'do not merge' label

Do we really need to do this? I figured that the existing CHANGELOG workflow would throw an exit 1, which will make the job fail and prevent merging as is. My questions was meant as more of a confirmation to validate my understanding.

If that's the case, I think its simpler to remove the workflow of adding the label, and then merge it in!

[h5law]

it will automatically add the 'do not merge' label

Do we really need to do this? I figured that the existing CHANGELOG workflow would throw an exit 1, which will make the job fail and prevent merging as is. My questions was meant as more of a confirmation to validate my understanding.

If that's the case, I think its simpler to remove the workflow of adding the label, and then merge it in!

I misunderstood the question sorry. I have removed the do not merge auto label job from the workflow. The pre-commit script that the workflow runs will exit with code 1 whenever there are any issues. It will log all the issues it finds not only the first one. This will cause the test to fail.

From my understanding whether or not a failed test prevents the PR being merged is decided in the repository settings. However I am not 100% on this, I am getting this from the documentation I linked in a previous message. I believe the exit 1 essentially does the same as the do not merge workflow that is already present so I am sure that you are right in thinking that adding the label is unnecessary.

I hope this is all alright 😃

[Olshansk]

I have removed the do not merge auto label job from the workflow

Appreciate you for bearing on the back-and-forth :)

From my understanding whether or not a failed test prevents the PR being merged is decided in the repository settings.

That is correct, and is the case for us.

However I am not 100% on this, I am getting this from the documentation I linked in a previous message

This isn't "mission critical" and I have high-confidence in the changes after the review.

If there's a small update/change we need to make after merging, would you be able to help us out?

I hope this is all alright 😃

🤝

[h5law]

If there's a small update/change we need to make after merging, would you be able to help us out?

Yeah of course, I hope I can get more involved anyway

[h5law]

To address those final commits - I realised the CHANGELOG.md file detection was wrong as the CHANGELOGs are stored in different sub directories (root of module, doc, docs) and so to address this when a CHANGELOG.md is detected in the path of a file the script will now add this path to a list and loop through that instead of trying to "build" the CHANGELOG.md path itself.

I also removed the dependency on the date command (past the basic usage that is portable across linux/osx/bsd) and am now comparing the dates from the changelog as a string against today's date.

Finally I have updated the relevant changelog in utility/doc/CHANGELOG.md as Jessica requested. Sorry for the large number of commits 😅

[Olshansk]

To address those final commits - I realised the CHANGELOG.md file detection was wrong as the CHANGELOGs are stored in different sub directories (root of module, doc, docs) and so to address this when a CHANGELOG.md is detected in the path of a file the script will now add this path to a list and loop through that instead of trying to "build" the CHANGELOG.md path itself.

Could you update the comment in the pre-commit if you think its relevant to add more details?

I also removed the dependency on the date command (past the basic usage that is portable across linux/osx/bsd) and am now comparing the dates from the changelog as a string against today's date.

Nice

Finally I have updated the relevant changelog in utility/doc/CHANGELOG.md as Jessica requested.

Left a comment about this.

Sorry for the large number of commits 😅

No worries assuming your okay with the back-and-forth! Let's get it right :)

Either way, what we do is a "squash & merge" where the git message is the PR description, so commit as many times as you want while working in progress.

[Olshansk]

We don't actually have a CHANGELOG for infra/tooling.

Putting this under build seems wrong (irrelevant), but creating a CHANGELOG under root also seems wrong (it's just tooling).

Since I don't have a good answer for it for now. Seems that your script checks for a CHANGELOG under root, which we don't have yet. DO you want to add it?

[h5law]

This was a question I posed in the original PR, I don't see the need to create a CHANGELOG simply for this one piece of tooling. I have removed the root directory check as there is nothing there. I think it is probably better in my opinion to remove this check as there isn't really a need for it.

If you want me to remove the changes I made to utility/doc/CHANGELOG.md I can but I don't see the point in making a new root level CHANGELOG.md as it probably will not see any updates besides this - there is no top-level code. All the other modules have their own as well so let me know what you think about this. I can if that's what you are looking for.

[h5law]

Adding the root changelog would mean that for any update to a module that requires go.mod or go.sum to change then both the module and root CHANGELOG.md files would need to be included - which would be an pointless repetition. We could get around this by having the script ignore all files in the root directory - as they don't require a changelog really. But this is the same as not checking it at all...

[Olshansk]

I don't see the need to create a CHANGELOG simply for this one piece of tooling

Agreed

. I think it is probably better in my opinion to remove this check as there isn't really a need for it.

Since I'm not 100% sure at the moment, but I can see us adding a root changelog soon (for main releases), can you add a ROOT_CHANGELOG_ENABLE=0 with a comment explaining that it should be enabled once a root CHANGELOG is added. We get to keep the code and document why it is off at the moment.

[h5law]

can you add a ROOT_CHANGELOG_ENABLE=0 with a comment explaining that it should be enabled once a root CHANGELOG is added. We get to keep the code and document why it is off at the moment.

So instead of this I have the script check the root level ONLY for a CHANGELOG.md file. This avoids having to do the full root level check/ignore certain files but allows for the root-level CHANGELOG.md - only when included to be validated.

This does mean there can never be an error situation for not including it in a commit. However it means that there doesn't need to be any code changes when the CHANGELOG gets implemented.

I would be happy to implement a checking system when the CHANGELOG is made and the circumstances defining when and when not to include it in a commit are clearly defined. But for now if its included it will be validated (date+version) like the rest but if not there will be no errors.

[Olshansk]

Left one comment here: https://github.com/pokt-network/pocket/pull/397/files#r1053724399

Wdyt of this compromise (best of both words w/ optionality) and then we'd be good to merge?

[Olshansk]

I don't see the need to create a CHANGELOG simply for this one piece of tooling

Agreed

. I think it is probably better in my opinion to remove this check as there isn't really a need for it.

Since I'm not 100% sure at the moment, but I can see us adding a root changelog soon (for main releases), can you add a ROOT_CHANGELOG_ENABLE=0 with a comment explaining that it should be enabled once a root CHANGELOG is added. We get to keep the code and document why it is off at the moment.

[h5law]

Left one comment here: https://github.com/pokt-network/pocket/pull/397/files#r1053724399

Wdyt of this compromise (best of both words w/ optionality) and then we'd be good to merge?

I have implemented something similar I explained the details in the reply to the comment above - but simply instead of having a hard "toggle" which would require an update when the CHANGELOG is implemented at root level the script now just checks for the presence of a root-level changelog and if its there it will validate it. No errors if its not there.

I have also removed the changes made to the utility/doc/CHANGELOG.md file.

[Olshansk]

Thank you @h5law and appreciate your collaboration on the back-and-forth. I learnt a few things along the way so lmk if you'd be interested in picking up more tasks in the future :)

As I mentioned earlier, we usually do a "Squash & Merge" + Put the PR description as the body so I will go ahead and do so!