Skip to content

ci: add codeQuality job to run lint and format task#3922

Merged
escapedcat merged 1 commit intoconventional-changelog:masterfrom
marcalexiei:feature/linting-ci
Feb 19, 2024
Merged

ci: add codeQuality job to run lint and format task#3922
escapedcat merged 1 commit intoconventional-changelog:masterfrom
marcalexiei:feature/linting-ci

Conversation

@marcalexiei
Copy link
Copy Markdown
Contributor

@marcalexiei marcalexiei commented Feb 17, 2024

Description

Add a new codeQuality job inside CI workflow.
The new job runs before build and nodeJsBaselineAptCompatibility.

Executed task are:

  1. yarn build
  2. yarn format
  3. yarn lint

This should avoid lints error addition if someone forgets to run yarn lint.

Motivation and Context

During #3850 I noticed that a lint issue fix was performed with a separate commit.

Basically config-nx-scopes was missing @commitlint/types dependency and running yarn lint shows that error.

You can see the same fix also on the fix lodash PR

Usage examples

N/A

How Has This Been Tested?

N/A

Types of changes

  • Github actions
  • 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)

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.

@marcalexiei marcalexiei changed the title ci: add codeQuality task to run lint and format task ci: add codeQuality job to run lint and format task Feb 17, 2024
@marcalexiei marcalexiei force-pushed the feature/linting-ci branch 2 times, most recently from eb90dd4 to ea4f7bb Compare February 17, 2024 07:56
@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Feb 17, 2024

Ohhh, wow, I was just wondering because I thought we already have this.
But we lost it when switching from CircleCI to Github Actions. Well, I forgot to migrate these...
Would you mind having a look if any of the other tasks should be re-added, like audit or deps (not sure what that actually does)

@marcalexiei
Copy link
Copy Markdown
Contributor Author

marcalexiei commented Feb 17, 2024 •
edited
Loading

Sure, no problem!
Would you like to have a new PR for the task "restoration", or I just keep updating this one?
(I personally prefer the first option to speed up things since the task of this PR is completed)

@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Feb 18, 2024

Just in here is fine, thank you so much!

knocte
knocte reviewed Feb 19, 2024
View reviewed changes
Comment thread .github/workflows/CI.yml Outdated
@marcalexiei
Copy link
Copy Markdown
Contributor Author

marcalexiei commented Feb 19, 2024

@escapedcat

audit task

are you sure do you want to run yarn audit inside CI?
Right now there 28 vulnerabilities:

  • 18 Moderate
  • 10 High

Besides that I think that dependabot / renovatebot will take care to open PR for this kind of issues when the fix is available.

deps task

This task which is present in every package runs a script inside @packages/utils folder:

https://github.com/conventional-changelog/commitlint/blob/master/%40packages/utils/dep-check.js

To be honest I'm not sure what is its purpose. Do you have any idea?

Right now is failing silently (err.exitCode is undefined so the process doesn't issue an error):

image

commitlint/@packages/utils/dep-check.js

Lines 8 to 13 in 2e225f7

return Promise.all([
check(['--missing', '--no-dev', '.']),
check(['--extra', '--no-dev', '.']),
])
.then((tasks) => [null, tasks.map((t) => t.stdout).join('\n')])
.catch((err) => [err]);

pkg task

Alongside deps task there is also a pkg task which performs some check on the output of yarn pack command.

This one is failing right now:

Screenshot 2024-02-19 at 09 28 50

Conclusion

If we have to restore pkg and deps tasks,
I strongly recommend to do this separately from this PR due to the fact that are not working right now.

knocte
knocte reviewed Feb 19, 2024
View reviewed changes
Comment thread .github/workflows/CI.yml Outdated
knocte
knocte reviewed Feb 19, 2024
View reviewed changes
Comment thread .github/workflows/CI.yml Outdated
@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Feb 19, 2024

If we have to restore pkg and deps tasks,
I strongly recommend to do this separately from this PR due to the fact that are not working right now.

Sure, let's do this.
We can think about if these are needed, same for the audit one. I mentioned these because those were existing before. We can reconsider if these add any value and should be re-added.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Feb 19, 2024

And the last nit from me is that, given that now codeQuality executes after build, then it's more logical to place its code after the build step, for readability. Other than that, LGTM

@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Feb 19, 2024

@marcalexiei wanna move the task below the build one as knocte suggested?
I guess then we're done with all the nitting (FTW!) and we can merge this.
Thanks for keep doing this!

btw, if any of you is interested you can join the new discord channel, which is is just empty currently:
#3522 (comment)

@marcalexiei
Copy link
Copy Markdown
Contributor Author

marcalexiei commented Feb 19, 2024

All changes have been applied.

@escapedcat escapedcat merged commit dccc081 into conventional-changelog:master Feb 19, 2024
@marcalexiei marcalexiei deleted the feature/linting-ci branch February 19, 2024 11:20
@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Feb 19, 2024

@knocte After switching everything to ESM and therefor to node v20 you know how to handle this?:
https://github.com/conventional-changelog/commitlint/actions/runs/7958768184/job/21724308968

escapedcat added a commit that referenced this pull request Feb 19, 2024
@escapedcat
Revert "ci: add codeQuality job to run lint and format tasks (#3922)"
927c39f 
This reverts commit dccc081.
@marcalexiei marcalexiei restored the feature/linting-ci branch February 19, 2024 12:43
escapedcat pushed a commit that referenced this pull request Feb 19, 2024
@marcalexiei @escapedcat
ci: add codeQuality job to run lint and format tasks (#3922)
59da18b
escapedcat pushed a commit that referenced this pull request Feb 27, 2024
@marcalexiei @escapedcat
ci: add codeQuality job to run lint and format tasks (#3922)
46c1b18
escapedcat added a commit that referenced this pull request Feb 27, 2024
@escapedcat
Revert "ci: add codeQuality job to run lint and format tasks (#3922)"
8bb94ca 
This reverts commit dccc081.
escapedcat pushed a commit that referenced this pull request Feb 27, 2024
@marcalexiei @escapedcat
ci: add codeQuality job to run lint and format tasks (#3922)
195078e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@escapedcat escapedcat escapedcat left review comments

+1 more reviewer

@knocte knocte knocte left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@marcalexiei @escapedcat @knocte