search

CommunitySolidServer / CommunitySolidServer

An open and modular implementation of the Solid specifications
https://communitysolidserver.github.io/CommunitySolidServer/
MIT License
521 stars 124 forks source link

Enforce conventional commits and enable semantic-release #145

Closed rubensworks closed 2 years ago

rubensworks commented 4 years ago

Even though we use conventional commits, this is not being enforced or checked. We could add a post-commit check for this, or some tool to help us write commit messages.

Once we get to 1.x.x we could make use of semantic-release to automatically determine the next package version and publish to npm.

MisterTimn commented 2 years ago

Where do we stand on this issue now?

I see some options

Option 1: Use commitlint and Husky for pre-commit hooks. So we enforce this on every commit.

Option 2: Use GH Action/App Semantic Pull Requests, this is actually quite nice in that it lives entirely on the repo or org and that it only enforces at least some conventional commits on the PR for automated changelogs/releases

From their readme:

Note! The default behavior of this bot is not to police all commit messages, but rather to ensure that every PR has just enough semantic information to be able to trigger a release when appropriate. The goal is to gather this semantic information in a way that doesn't make life harder for project contributors, especially newcomers who may not know how to amend their git commit history. By default, only the PR title OR at least one commit message needs to have semantic prefix.

That's for the conventional commit enforcing part mainly, as for automated releases, I think I need to direct the question @joachimvh ? Is this something we want right now / ever?

Also related (as we are talking about commit hooks and developer experience), right now there is a pre-commit hook which runs the full test suite and linter. But I think most people run their tests manually or through extensions (vscode-jest for example) so when committing they either run the same tests that have already passed again, or just add --no-verify to skip it. Since we already protect main and work through PRs that get tested on each push, I would just remove this pre-commit hook, save for linting maybe because the linter works quite fast.

joachimvh commented 2 years ago

Option 2 doesn't seem that useful to me since it doesn't solve the problem of having all commits be in the expected format. Unless we make it very strict, but I don't want users to have their PR rejected because they used an invalid commit format. That would cause a lot of hassle for them. Option 1 does look interesting though since that way it prevents invalid commits from being created in the first place. This might cause confusion for users though, depending on the error thrown by husky, so open to other opinions there. It's not that big of a deal if someone uses a different commit format since that can always be changed by a squash commit in a PR.

That's for the conventional commit enforcing part mainly, as for automated releases, I think I need to direct the question @joachimvh ? Is this something we want right now / ever?

This is the current process when doing a release: https://github.com/CommunitySolidServer/CommunitySolidServer/blob/main/documentation/release.md

If some more steps can be automated there (without potentially introducing errors that can be missed). Formatting CHANGELOG.md is always going to be a manual step, so a fully automated flow is not possible. If anything is possible there it would be nice, but not a big issue if not.

Also related (as we are talking about commit hooks and developer experience), right now there is a pre-commit hook which runs the full test suite and linter. But I think most people run their tests manually or through extensions (vscode-jest for example) so when committing they either run the same tests that have already passed again, or just add --no-verify to skip it. Since we already protect main and work through PRs that get tested on each push, I would just remove this pre-commit hook, save for linting maybe because the linter works quite fast.

I would keep it. Like you mentioned, you can choose to disable them if you know what you're doing, but I like that the default is that they are being run. If someone has less of an idea of what they're doing, because they're less experienced with the ecosystem, they might not run tests before doing a PR. This step increases the chances of a PR being valid immediately. Besides that it has also already prevented me several times from committing mistakes after I made a small change that I assumed would not have an impact 😄

MisterTimn commented 2 years ago

I added commit-lint and enabled the commit-msg hook for it. Slight problem is that --no-verify ofcourse bypasses that hook as well. But assuming people who use that flag know what they are doing, this should be fine :P

image

This uses the config-conventional configuration, we can add our own custom config if needed, but this covers our current commit messages I believe

If needed we can lint the commit msg in CI as well, perhaps only when merging the PR into main or version/* so we enforce the squashed commit to adhere to conventions.

MisterTimn commented 2 years ago

Option 2 doesn't seem that useful to me since it doesn't solve the problem of having all commits be in the expected format. Unless we make it very strict, but I don't want users to have their PR rejected because they used an invalid commit format.

Actually, 2 would only enforce having at least a semantic PR title OR at least 1 conventional commit. The CI step would have a status symbol like so. So new developers can start their PR without using conv commits and then refer to the status and make necessary changes (either editing PR title or adding a conventional commit). image

Option 1 with commitlint as implemented now will check on every commit (except when --no-verify is used) so will be more of a blocker for new developers. I believe the error message is descriptive enough though and I think we can even add our own links in the output and refer to our own developer readme with a section regarding conv commits.

joachimvh commented 2 years ago

The commit-lint looks nice. It links to the documentation so that should be enough. Does it also support commits such as fix(deps): ... such as renovatebot uses? E.g. https://github.com/CommunitySolidServer/CommunitySolidServer/pull/1203

I don't see the CI linting working since once it's in the main or version/* branch it's too late since we don't force push on those branches.

For the PRs, the title don't have to be in the conventional commit style (mine usually aren't unless it's a small PR). And only having some of the commits be correct also doesn't help me that much. And I don't want users who are less familiar with git to have to rebase their PR.

MisterTimn commented 2 years ago

The commit-lint looks nice. It links to the documentation so that should be enough. Does it also support commits such as fix(deps): ... such as renovatebot uses? E.g. #1203

Yes, scoping is supported (but not enforced).

MisterTimn commented 2 years ago

This is the current process when doing a release: https://github.com/CommunitySolidServer/CommunitySolidServer/blob/main/documentation/release.md

If some more steps can be automated there (without potentially introducing errors that can be missed). Formatting CHANGELOG.md is always going to be a manual step, so a fully automated flow is not possible. If anything is possible there it would be nice, but not a big issue if not.

FYI, have started on some scripts to use in CI/turn into actions to perform some of these release steps automatically. Changing the tag in config files and lsd entries on package.json already done.

For the changelog, I'm thinking we can automate much of it and then hand it over to someone to manually parse and format. This could be something custom which performs the reversing and ordering of commit messages, or we can use a lib or action.

I tried out standard-version which handles conventional commits. It's not perfect but could be a big help, especially if we tweak the changelog config to adhere more to our style + if we start using prefix(scope): text for our squashed PR commits. To give an idea, this is its output (added to changelog.md) for release 4.0.0 (which it itself would release as 3.1.0 because it doesn't detect the breaking changes).

3.1.0 (2022-05-03)

Features

Bug Fixes

joachimvh commented 2 years ago

FYI, have started on some scripts to use in CI/turn into actions to perform some of these release steps automatically. Changing the tag in config files and lsd entries on package.json already done.

How exactly does this work with CI? What would cause this to trigger? One issue is that the chore: Update configs to vx.0.0 commit is actually invalid and would not pass CI since this makes the configs invalid (since they're not matching the version number).

For the changelog, I'm thinking we can automate much of it and then hand it over to someone to manually parse and format. This could be something custom which performs the reversing and ordering of commit messages, or we can use a lib or action.

If the reversing and ordering could be automated that would be nice already. Although currently the ordering doesn't always strictly follow the conventional tags, but I'm going to assume nobody really cares that much if the categories are reduced in the changelog.

Also already want to note that if we have local scripts these should preferably be JS/TS and not bash scripts.

if we start using prefix(scope): text for our squashed PR commits.

This should already be the case, it's one of the reasons I sometimes squash. And with the pre-commit check on the commit message this should be less of an issue.

Had a look at the readme of manual-git-changelog that we're using and it states "This is an alternative to tools such as conventional-changelog for projects that don't follow a specific commit convention, but wanting to automate your CHANGELOG.md generation upon each release.". So would contentional-changelog-cli be an option then since we do use a contentional commits?

MisterTimn commented 2 years ago

Had a look at the readme of manual-git-changelog that we're using and it states "This is an alternative to tools such as conventional-changelog for projects that don't follow a specific commit convention, but wanting to automate your CHANGELOG.md generation upon each release.". So would contentional-changelog-cli be an option then since we do use a contentional commits?

Ah, yes seems to me that's a better fit than the standard version I've linked. It just does the changelog and nothing more.

How exactly does this work with CI? What would cause this to trigger?

Not sure on the trigger, either we trigger more releases by triggering on main pushes (minor and patch releases) or maybe we use a PR for releases, detecting that we are releasing through labels/PR title for example.

One issue is that the chore: Update configs to vx.0.0 commit is actually invalid and would not pass CI since this makes the configs invalid (since they're not matching the version number).

We can skip CI for those commits by committing chore: Update configs to vx.0.0 [skip ci] (https://github.blog/changelog/2021-02-08-github-actions-skip-pull-request-and-push-workflows-with-skip-ci/) or just leave this as part of a script we use locally.

Just to give an idea, a possible scenario below, going from the release.md entries. This would be triggered from a release PR of versions/x.0.0 into main

Then we could trigger on a merge into main/tags to handle the rest of the release procedure or keep it in the one flow (need to try this out first)

I think the release PR with the new changelog and the componentsjs references would be a nice start. The testing after merging main, is this something you do manually now @joachimvh or is this using the unit and integration tests?

MisterTimn commented 2 years ago

if we start using prefix(scope): text for our squashed PR commits.

This should already be the case, it's one of the reasons I sometimes squash. And with the pre-commit check on the commit message this should be less of an issue.

If we also start using feat!: text and/or footers with BREAKING CHANGES we can also get correct automated semantic version bumps :) https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with-scope-and--to-draw-attention-to-breaking-change

joachimvh commented 2 years ago

or just leave this as part of a script we use locally.

I prefer doing most of it locally instead of through CI. I prefer there being a human check at the end to verify everything.

The testing after merging main, is this something you do manually now @joachimvh or is this using the unit and integration tests?

This is all manual. I set up the demoClientApp from the authn library to verify browser-based login and experiment what happens when upgrading the server, which is how I discovered 570e167a36b8da14561680d2709190ea78e15b61 just before release for example.

Upgrading the recipes automatically is also something that is only going to work sometimes, since if a new import is added in the default configs, it will also have to be added there for example.

If we also start using feat!: text and/or footers with BREAKING CHANGES we can also get correct automated semantic version bumps :)

While that might be a nice to have, we already sort of have this based on the branch that is being targeted. And I doubt this will always be used correct consistently. 😄

Just want to note here that the main idea was that if some steps can be automated in the process, that is nice to have, but I still want there to be some manual checks in there (e.g., remove irrelevant commits from the changelog, verify that upgrading works).

MisterTimn commented 2 years ago

Fair 😃 So the first half preferably local, then everything can be checked manually. The last half can be done quickly with automation.

So then things to add for local usage

Then we can trigger on tag/merge into to main to do the github release, npm publish, change the versions entries in workflow file and dependabot.yml, branch renaming + other housekeeping.

joachimvh commented 2 years ago

Sounds good

rubensworks commented 2 years ago

A JS/TS script to update componentsjs references that can be called locally as part of npm release

It might make sense to include this as a script in Components.js directly even, since all projects making use of Components.js would run into this problem.

MisterTimn commented 2 years ago

@rubensworks any pointers on how to best go about this? This would mean a contribution to componentsjs which adds a command that can be used along with npm version?

Possible issue now, that might be OK for our project but not necessarily for other is the following: When using npm version now two commits are created with the following contents

Config commit

- All components config found like configs/**/*.json

Release commit

- package.json (version bump + the @solid/community-server config changes )
- package-lock.json
- changelog.md

Since both steps (npm version itself and config version changes) edit package.json you get this slightly weird git commit history. Using preversion so you can commit separately would make the script way more complicated I think, as it has no way of figuring out what version bump is requested (and in our case the reference only change on majors)

rubensworks commented 2 years ago

@rubensworks any pointers on how to best go about this? This would mean a contribution to componentsjs which adds a command that can be used along with npm version?

@MisterTimn I would keep all of the commit logic here in CSS, since that may differ per project. I think only the logic for migrating context URLs in config files would make sense to add to CJS.

We could add a JS bin script for that here: https://github.com/LinkedSoftwareDependencies/Components.js/tree/master/bin (I guess that script will only require a few lines of code)

We then also would have to expose that bin script here, so that other can use it after installing CJS: https://github.com/LinkedSoftwareDependencies/Components.js/blob/master/package.json#L10

joachimvh commented 2 years ago

Done in #1285 and #1294