docs: fix github actions ci example #3892

letsaguiar · 2024-02-01T20:44:40Z

Description

This PR updates the github actions CI example. The previous one doesn't works by itself and has some unnecessary steps, so these changes turns the example CI more solid and developer friendly.

Motivation and Context

When I try to run the provided github CI example in a personal project it always returns the following error:

Run npx commitlint --from HEAD~1 --to HEAD --verbose
/home/runner/work/eslint-config/eslint-config/node_modules/commitlint/node_modules/@commitlint/cli/lib/cli.js:123
        throw err;
        ^

Error: fatal: ambiguous argument 'HEAD~1..HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

    at Transform._transform (/home/runner/work/eslint-config/eslint-config/node_modules/git-raw-commits/index.js:8[4](https://github.com/pitagoras-educacao/eslint-config/actions/runs/7746930452/job/21126268440#step:5:5):30)
    at Transform._read (/home/runner/work/eslint-config/eslint-config/node_modules/readable-stream/lib/_stream_transform.js:166:10)
    at Transform._write (/home/runner/work/eslint-config/eslint-config/node_modules/readable-stream/lib/_stream_transform.js:1[5](https://github.com/pitagoras-educacao/eslint-config/actions/runs/7746930452/job/21126268440#step:5:6)5:83)
    at doWrite (/home/runner/work/eslint-config/eslint-config/node_modules/readable-stream/lib/_stream_writable.js:390:139)
    at writeOrBuffer (/home/runner/work/eslint-config/eslint-config/node_modules/readable-stream/lib/_stream_writable.js:381:5)
    at Transform.Writable.write (/home/runner/work/eslint-config/eslint-config/node_modules/readable-stream/lib/_stream_writable.js:302:11)
    at Socket.ondata (internal/streams/readable.js:731:22)
    at Socket.emit (events.js:412:35)
    at addChunk (internal/streams/readable.js:293:12)
    at readableAddChunk (internal/streams/readable.js:2[6](https://github.com/pitagoras-educacao/eslint-config/actions/runs/7746930452/job/21126268440#step:5:7)3:[11](https://github.com/pitagoras-educacao/eslint-config/actions/runs/7746930452/job/21126268440#step:5:12))
Error: Process completed with exit code 1.

This happens because we have to manually checkout the repository with the ref tag on actions/checkout actions. Otherwise, there is going to be no HEAD in the container running our pipeline. This is the main change.

Upgrade default container image to ubuntu-latest
Upgrade actions/checkout from v3 to v4
Avoid unnecessary steps using actions/setup-node github action
Install commitlint globally and use the scoped distribution

How Has This Been Tested?

I've used the exact same implementation on a personal project and it worked

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
[ x ] Documentation improvements

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

docs/guides-ci-setup.md

knocte · 2024-02-02T04:22:53Z

@letsaguiar sorry if the above review comes off as harsh, PRs upgrading documentation are appreciated! thanks for cooking this PR

letsaguiar · 2024-02-02T10:09:45Z

@letsaguiar sorry if the above review comes off as harsh, PRs upgrading documentation are appreciated! thanks for cooking this PR

Don't worry, thank you for the insights :D I've updated the PR according to your requests

knocte · 2024-02-02T13:02:06Z

docs/guides-ci-setup.md

-          git --version
-          node --version
-          npm --version
-          npx commitlint --version


@letsaguiar and why dont you like printing versions 😭 😅

Short answer: I don't think it's useful since we already know which version of git comes with Ubuntu 22.04 and which version of node we're installing

Long answer: an unnecessary step makes the pipeline slow and increase the cloud costs

those points apply to git and node, but not to the other 2

btw, your long answer is shorter than your short answer 😆

But the meaning is deeper haha

I'm going to revert it :D

sounds good

knocte · 2024-02-04T03:05:29Z

docs/guides-ci-setup.md

-        run: |
-          npm install conventional-changelog-conventionalcommits
-          npm install commitlint@latest
+        run: npm install --global @commitlint/{cli,config-conventional}


@letsaguiar are you sure this syntax is correct? I've tried it myself in a repo of mine because I thought it would be nice to adopt it, and I'm getting this error:

Run npm install --global @commitlint/{cli,config-conventional} npm ERR! code ENOENT npm ERR! syscall open npm ERR! path /__w/conventions/conventions/@commitlint/{cli,config-conventional}/package.json npm ERR! errno -2 npm ERR! enoent ENOENT: no such file or directory, open '/__w/conventions/conventions/@commitlint/{cli,config-conventional}/package.json' npm ERR! enoent This is related to npm not being able to find a file. npm ERR! enoent npm ERR! A complete log of this run can be found in: /github/home/.npm/_logs/202[4](https://github.com/knocte/conventions/actions/runs/7771223169/job/21192285282#step:6:5)-02-04T02_[5](https://github.com/knocte/conventions/actions/runs/7771223169/job/21192285282#step:6:6)[9](https://github.com/knocte/conventions/actions/runs/7771223169/job/21192285282#step:6:10)_54_919Z-debug-0.log Error: Process completed with exit code 254.

(conventions is the name of my repo btw)

Interesting thing is that the above syntax works when you're running your CI job without a container: attribute, but fails with the above message when using container: (a docker image). I have no idea why this happens. But this needs to be reverted from the PR in case the commitlint user reading the documentation is using container images instead of default github VMs.

knocte · 2024-02-06T07:21:10Z

Error: fatal: ambiguous argument 'HEAD~1..HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:

I just reproduced this problem myself, and the culprit is actually because the git repository only has 1 commit (the initial commit). In this case, walking backwards to HEAD~1 may not be supported. Are we sure the ref: attribute in this PR fixes this situation?

knocte · 2024-02-06T07:21:32Z

because the git repository only has 1 commit (the initial commit)

So, the workaround is just push a 2nd commit, and then it works.

escapedcat · 2024-02-06T15:00:24Z

Are we good here?

knocte · 2024-02-06T17:55:08Z

Are we good here?

Not yet, she said she would revert something which she didn't yet; and after that I raised 2 concerns.

primeare · 2024-02-08T13:55:42Z

I've also been thinking about changing the GitHub Actions CI example in this repo. Fortunately, before contributing, I checked existing PRs. It's awesome to find other contributors working on the same issue! 🚀
I want to introduce some additions and changes to this proposal:

- name: Validate commits
  if: contains(fromJSON('["push", "pull_request"]'), github.event_name)
  env:
    FROM: ${{ github.event.before || github.event.pull_request.base.sha }}
    TO: ${{ github.event.after || github.event.pull_request.head.sha }}
  run: npx commitlint --from $FROM --to $TO --verbose

This commitlint execution step has some exciting benefits:

for a push event, it not only checks the latest commit but all commits that were pushed since the last CI run;
for a pull_request event, it utilises github context to use base and head commit hashes;
it combines those events into one universal step.

We can also recommend installing @commitlint/cli and @commitlint/config-conventional locally to end-users' repositories. That is considered a good industry practice to use a per-project commitlint version. Although we probably want to encourage users to follow Conventional Commits, it is worth noting in the GitHub Action CI example that users are free to choose any convention.

knocte · 2024-02-10T05:54:17Z

This commitlint execution step has some exciting benefits:

Very interesting! Do you know if it works also on repos with just 1 commit?

primeare · 2024-02-10T21:31:58Z

This commitlint execution step has some exciting benefits:

Very interesting! Do you know if it works also on repos with just 1 commit?

Sure, I've tested this step on one of my projects, and it works in all scenarios, including a single commit.

knocte · 2024-02-11T06:18:41Z

Sure, I've tested this step on one of my projects, and it works in all scenarios, including a single commit.

Are you 100% sure? I haven't tested it yet but AFAIU in a single-commit scenario both $FROM and $TO would map to the same commit hash, which then would make commit-lint not crash (which, granted, is better than crashing) but would actually not do anything, at least according to this bug that I filed some months ago: #3376

viveleroi · 2024-02-11T06:31:34Z

I can confirm that the ref fix in this PR solves the ambiguous argument 'HEAD~1..HEAD' issue for us. I added this to an existing lint actions yml file that used actions/checkout@v4 but hadn't used the other parts of the example file so I didn't adjust those with these changes.

knocte · 2024-02-11T06:33:47Z

I can confirm that the ref fix in this PR solves the ambiguous argument 'HEAD~1..HEAD' issue for us.

Thanks for your input @viveleroi ; the problem I have with that solution is that it may work for a pull_request trigger, but not for a push one.

viveleroi · 2024-02-11T06:36:42Z

I can confirm that the ref fix in this PR solves the ambiguous argument 'HEAD~1..HEAD' issue for us.

Thanks for your input @viveleroi ; the problem I have with that solution is that it may work for a pull_request trigger, but not for a push one.

I'm just starting this repo so I've only been using pushes. That fix solved pushes for me. I know the ref has the pull_request name in it, but I have yet to open a PR so somehow that was acceptable here.

knocte · 2024-02-12T05:56:34Z

That fix solved pushes for me

But I guess the issue was only happening when the repo had one single commit, right?

viveleroi · 2024-02-12T16:06:04Z

Do you mean the push or the entire repo? I was pushing to a relatively new repo. It had one commit already when I first added an action, but didn't include commitlint in that work. So by the time I pushed this commitlint action I had 2 commits in the repo, 1 commit in my push. The action failed. I pushed again with the fix above and it succeeded - with 3 commits in the repo and and 1 push. None of these were via PR

primeare · 2024-02-12T19:36:29Z

Sure, I've tested this step on one of my projects, and it works in all scenarios, including a single commit.

Are you 100% sure? I haven't tested it yet but AFAIU in a single-commit scenario both $FROM and $TO would map to the same commit hash, which then would make commit-lint not crash (which, granted, is better than crashing) but would actually not do anything, at least according to this bug that I filed some months ago: #3376

From the GitHub Actions documentation, push event's before points to the most recent commit on ref before the push and after points to the most recent commit on ref after the push. I tested that on a single commit in one of my repositories, and it works:

knocte · 2024-02-13T03:13:04Z

@primeare if you say your cropped screenshot comes from an execution on a repo that only has 1 commit, then why are there 2 commit hashes?

primeare · 2024-02-13T15:26:44Z

Sorry, @knocte, I misunderstood you. I tested this workflow with the initial single commit, and you are right. It does not work because GitHub Actions uses zero commit hash as a previous commit:

We could cover these two edge cases in commitlint: one for the zero commit hash and if the FROM and TO are equal. Both should omit then FROM and only use TO. What do you think about that?

primeare · 2024-02-13T17:56:17Z

Also, my proposed workflow step does not work with force push: GitHub Actions provide the hash of the previous commit in before that does not exist anymore.

In a single-commit repo, this would lead to this error: ``` Error: fatal: ambiguous argument 'HEAD~1..HEAD': unknown revision or path not in the working tree. ``` Now that we have the `--last` flag, this should work for the case in which we want just to analyze the last commit, not specify a range of commits via `--from` and `--to`. Closes conventional-changelog#3892

knocte · 2024-02-14T06:26:30Z

We could cover these two edge cases in commitlint: one for the zero commit hash and if the FROM and TO are equal. Both should omit then FROM and only use TO. What do you think about that?

Mmm, how about this other idea (which looks cleaner to me):

#3916

Rather use more straightforward package names, as advised in [1] and [2]. [1] conventional-changelog/commitlint#3892 [2] Kochava/github-workflows#30

In a single-commit repo, this would lead to this error: ``` Error: fatal: ambiguous argument 'HEAD~1..HEAD': unknown revision or path not in the working tree. ``` Now that we have the `--last` flag, this should work for the case in which we want just to analyze the last commit, not specify a range of commits via `--from` and `--to`. Closes conventional-changelog#3892

) * test(cli): add regression test * feat(cli): implement new `--last` CLI flag The upgrade of ES2017 to ES2022 is because of the use of `Object.hasOwn` API which is less ugly than hardcoding `undefined`. * docs(ci): stop recommending HEAD~1 In a single-commit repo, this would lead to this error: ``` Error: fatal: ambiguous argument 'HEAD~1..HEAD': unknown revision or path not in the working tree. ``` Now that we have the `--last` flag, this should work for the case in which we want just to analyze the last commit, not specify a range of commits via `--from` and `--to`. Closes #3892 * fix: use --pretty=format:"%B" instead of --pretty=%B The latter adds unnecessary whitespace at the end of the commit msg. --------- Co-authored-by: Mehrshad <code.rezaei@gmail.com>

docs: fix github actions ci example

be2bc76

knocte reviewed Feb 2, 2024

View reviewed changes

docs/guides-ci-setup.md Outdated Show resolved Hide resolved

knocte reviewed Feb 2, 2024

View reviewed changes

docs/guides-ci-setup.md Outdated Show resolved Hide resolved

knocte reviewed Feb 2, 2024

View reviewed changes

docs/guides-ci-setup.md Outdated Show resolved Hide resolved

docs: revert some changes

395a14c

knocte reviewed Feb 2, 2024

View reviewed changes

knocte reviewed Feb 4, 2024

View reviewed changes

escapedcat requested a review from knocte February 6, 2024 15:00

knocte mentioned this pull request Feb 14, 2024

feat(cli): introduce new --last flag, to stop recommending HEAD~1 #3916

Merged

knocte added a commit to nblockchain/conventions that referenced this pull request Feb 15, 2024

commitlint.sh: simplify

f1e8c14

Rather use more straightforward package names, as advised in [1] and [2]. [1] conventional-changelog/commitlint#3892 [2] Kochava/github-workflows#30

knocte added a commit to nblockchain/conventions that referenced this pull request Feb 16, 2024

commitlint.sh: simplify

e261cd1

Rather use more straightforward package names, as advised in [1] and [2]. [1] conventional-changelog/commitlint#3892 [2] Kochava/github-workflows#30

escapedcat closed this in #3916 Mar 14, 2024

letsaguiar deleted the fix/github_actions_ci_example branch April 5, 2024 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: fix github actions ci example #3892

docs: fix github actions ci example #3892

letsaguiar commented Feb 1, 2024 •

edited

Loading

knocte commented Feb 2, 2024

letsaguiar commented Feb 2, 2024

knocte Feb 2, 2024

letsaguiar Feb 2, 2024

knocte Feb 2, 2024

knocte Feb 2, 2024

letsaguiar Feb 2, 2024

letsaguiar Feb 2, 2024

knocte Feb 4, 2024

knocte Feb 4, 2024 •

edited

Loading

knocte Feb 15, 2024

knocte commented Feb 6, 2024

knocte commented Feb 6, 2024

escapedcat commented Feb 6, 2024

knocte commented Feb 6, 2024

primeare commented Feb 8, 2024

knocte commented Feb 10, 2024

primeare commented Feb 10, 2024

knocte commented Feb 11, 2024

viveleroi commented Feb 11, 2024

knocte commented Feb 11, 2024

viveleroi commented Feb 11, 2024

knocte commented Feb 12, 2024

viveleroi commented Feb 12, 2024

primeare commented Feb 12, 2024

knocte commented Feb 13, 2024

primeare commented Feb 13, 2024

primeare commented Feb 13, 2024

knocte commented Feb 14, 2024

docs: fix github actions ci example #3892

docs: fix github actions ci example #3892

Conversation

letsaguiar commented Feb 1, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

knocte commented Feb 2, 2024

letsaguiar commented Feb 2, 2024

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

knocte Feb 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knocte commented Feb 6, 2024

knocte commented Feb 6, 2024

escapedcat commented Feb 6, 2024

knocte commented Feb 6, 2024

primeare commented Feb 8, 2024

knocte commented Feb 10, 2024

primeare commented Feb 10, 2024

knocte commented Feb 11, 2024

viveleroi commented Feb 11, 2024

knocte commented Feb 11, 2024

viveleroi commented Feb 11, 2024

knocte commented Feb 12, 2024

viveleroi commented Feb 12, 2024

primeare commented Feb 12, 2024

knocte commented Feb 13, 2024

primeare commented Feb 13, 2024

primeare commented Feb 13, 2024

knocte commented Feb 14, 2024

letsaguiar commented Feb 1, 2024 •

edited

Loading

knocte Feb 4, 2024 •

edited

Loading