Skip to content
This repository was archived by the owner on Feb 11, 2025. It is now read-only.

Enforce commits are following our conventions via github actions#76

Merged
peanball merged 2 commits intocloudfoundry:mainfrom
sap-contributions:commitlint-github-action
Jul 24, 2023
Merged

Enforce commits are following our conventions via github actions#76
peanball merged 2 commits intocloudfoundry:mainfrom
sap-contributions:commitlint-github-action

Conversation

@raghunathd8
Copy link
Copy Markdown
Contributor

@raghunathd8 raghunathd8 commented Jul 6, 2023

Adds a commitlint check job that ensures the following rules:

  1. conventional commits
  2. supported commit types: feat, fix, dep, doc, ci, refactor, test
  3. Breaking changes can be indicated with a ! before :, e.g. feat!: new breaking feature.
    Breaking changes must have a breaking-change: or breaking change: trailer. This is case in-sensitive.

The commitlint configuration ensures those settings. The GitHub Actions job executes them and fails the PR validation if any of the commits in a PR are not compliant. Those can then be amended as needed by the PR author.

@raghunathd8 raghunathd8 requested a review from a team July 6, 2023 13:58
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Jul 6, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: peanball / name: Alexander Lais (0e532d3)
  • ✅ login: raghunathd8 / name: Raghunath Deshpande (d16d02a, d16d02a)
  • ✅ login: Soha-Albaghdady / name: Soha Alboghdady (d16d02a)

@raghunathd8 raghunathd8 force-pushed the commitlint-github-action branch from 8906c8c to 8322e87 Compare July 6, 2023 14:00
Comment thread .github/workflows/commitlint.yml Outdated
@peanball
Copy link
Copy Markdown
Contributor

peanball commented Jul 6, 2023

The trailer is in the wrong format in the commit, should be:

Co-authored-by: Soha Alboghdady <soha.alboghdady@sap.com>

< vs. (.

@Soha-Albaghdady Soha-Albaghdady force-pushed the commitlint-github-action branch 3 times, most recently from 78afc10 to 7ed216a Compare July 7, 2023 11:25
@plowin plowin added the run-ci Triggers PR Validation label Jul 7, 2023
@maxmoehl
Copy link
Copy Markdown
Member

maxmoehl commented Jul 7, 2023

@plowin if you want to run the CI you have to approve the PR ;)

Comment thread .github/workflows/commitlint.yml Outdated
peanball
peanball previously approved these changes Jul 11, 2023
@raghunathd8 raghunathd8 force-pushed the commitlint-github-action branch from 8eb6a69 to 7fe20f4 Compare July 11, 2023 06:34
@peanball
Copy link
Copy Markdown
Contributor

peanball commented Jul 11, 2023

@Soha-Albaghdady it seems that GitHub actions is picky about what it supports as base image on public runners. Right now all PRs are blocked because none of the commitlint steps run.

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources

See also https://stackoverflow.com/questions/70959954/error-waiting-for-a-runner-to-pick-up-this-job-using-github-actions

So maybe we need to change back to ubuntu-latest and install npm.

There is an action for that though, that allows you to select the NPM version. Action resources should also be cached and should be the preferred way over downloading via script on the fly.

https://github.com/actions/setup-node

@raghunathd8
Copy link
Copy Markdown
Contributor Author

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources

This is exactly what I was referring to, github have selected specific runners that they support and encourage to use.

@peanball
Copy link
Copy Markdown
Contributor

This is exactly what I was referring to, github have selected specific runners that they support and encourage to use.

It seems that they just don't support others than the listed ones.

@peanball
Copy link
Copy Markdown
Contributor

one final bit for the commit, as this is a change on the overall CI setup, I would propose the prefix ci: .

@Soha-Albaghdady Soha-Albaghdady force-pushed the commitlint-github-action branch from 16c544a to 7ab869d Compare July 11, 2023 09:16
Copy link
Copy Markdown
Contributor

@peanball peanball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the checkout with depth 0 may be counteracting the commit lint checking of a commit range. Could you please clarify?

Comment thread .github/workflows/commitlint.yml
Comment thread .github/workflows/commitlint.yml Outdated
@Soha-Albaghdady Soha-Albaghdady force-pushed the commitlint-github-action branch 2 times, most recently from 306cddf to 1fe29ec Compare July 11, 2023 13:36
@peanball peanball force-pushed the commitlint-github-action branch 2 times, most recently from ec5551f to 65c8d49 Compare July 13, 2023 15:33
@peanball
Copy link
Copy Markdown
Contributor

Checked with "breaking change" indicator ! without the appropriate header in action build https://github.com/cloudfoundry/pcap-release/actions/runs/5545026407/jobs/10123338288?pr=76

works.

@peanball peanball dismissed their stale review July 14, 2023 05:32

changes applied and ok. I applied further changes that I don't want to approve myself.

@peanball peanball changed the title test: commitlint github actions Enforce commits are following our conventions via github actions Jul 14, 2023
Comment thread commitlint.config.js
@peanball peanball force-pushed the commitlint-github-action branch from 65c8d49 to c9fd84f Compare July 17, 2023 05:52
@peanball peanball requested a review from b1tamara July 17, 2023 05:53
@peanball peanball force-pushed the commitlint-github-action branch from c9fd84f to fce5cb6 Compare July 17, 2023 07:19
Copy link
Copy Markdown
Contributor

@a18e a18e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments/suggestions, otherwise great!

Comment thread .github/workflows/commitlint.yml Outdated
Comment thread .github/workflows/commitlint.yml Outdated
Comment thread commitlint.config.js Outdated
Comment thread commitlint.config.js Outdated
Comment thread commitlint.config.js Outdated
Comment thread commitlint.config.js Outdated
peanball and others added 2 commits July 20, 2023 17:25
* breaking change handler
* type enum according to our convention
Co-authored-by: Soha Alboghdady <soha.alboghdady@sap.com>
Co-authored-by: Raghunath Deshpande <raghunath.deshpande@sap.com>
@peanball peanball force-pushed the commitlint-github-action branch from fce5cb6 to d16d02a Compare July 20, 2023 15:28
@peanball peanball requested review from Soha-Albaghdady, a18e and peanball and removed request for Soha-Albaghdady and peanball July 21, 2023 05:25
Copy link
Copy Markdown
Contributor

@b1tamara b1tamara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

@a18e a18e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you omit the space before the subject (e.g. dep(scope)!:Subjectstarts), commitlint will fail with the error type may not be empty.
I find this confusing/misleading.
The following suggestions fix this but in a slightly hacky way (also the rule name breaking-change-indicator doesn't fit anymore).
I'm fine with the suggestion being ignored and approve of the rest of the PR.

Comment thread commitlint.config.js
Comment thread commitlint.config.js
@peanball
Copy link
Copy Markdown
Contributor

If you omit the space before the subject (e.g. dep(scope)!:Subjectstarts), commitlint will fail with the error type may not be empty.
I find this confusing/misleading.
The following suggestions fix this but in a slightly hacky way (also the rule name breaking-change-indicator doesn't fit anymore).
I'm fine with the suggestion being ignored and approve of the rest of the PR.

As mentioned on the other comment, it's too hacky for me. Also for the purpose of our initial goal, i.e. "don't allow misfit commits", this does not add anything.

If you wanted, you can propose a new checker (via separate PR) that does more than type-empty from the default set and inspects the commit message and all its elements in full. But I see this separate to the breaking change indicator, and a non-vital improvement.

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

Labels

run-ci Triggers PR Validation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants