New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: migrate linting workflow to use trunk check metalinter #17876
base: main
Are you sure you want to change the base?
Conversation
Hi @EliSchleifer!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
|
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @EliSchleifer!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
Thanks for putting this together. It looks like the CI is failing with a message "trunk not found". Is there a way to get this working in CI to see what the output looks like? We're working on getting our v9.0.0 alpha out this week, so will have more time to investigate this next week. |
Will get that fixed. I’m on vacation till end of the week
…________________________________
From: Nicholas C. Zakas ***@***.***>
Sent: Wednesday, December 27, 2023 11:06:22 AM
To: eslint/eslint ***@***.***>
Cc: Eli Schleifer ***@***.***>; Mention ***@***.***>
Subject: Re: [eslint/eslint] chore: migrate linting workflow to use trunk check metalinter (PR #17876)
Thanks for putting this together. It looks like the CI is failing with a message "trunk not found". Is there a way to get this working in CI to see what the output looks like?
We're working on getting our v9.0.0 alpha out this week, so will have more time to investigate this next week.
—
Reply to this email directly, view it on GitHub<#17876 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAJVCPVJGWDYDW6UV4GCXBLYLRWS5AVCNFSM6AAAAABA2WKCSOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZQGU2TQMZUGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
.trunk/trunk.yaml
Outdated
- name: lint | ||
run: ${workspace}/bin/eslint.js --output-file ${tmpfile} --format json ${target} | ||
enabled: | ||
- eslint@8.56.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if this is needed given the previous definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - the definition teaches trunk how to run a tool - and the enabled list turns on the tools.
.trunk/trunk.yaml
Outdated
- trunk-check-pre-push | ||
- trunk-fmt-pre-commit | ||
enabled: | ||
- trunk-upgrade-available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Will something like Renovate know to upgrade trunk? Or is this something that needs to happen manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently upgrading is manual - we are working on a PR to make renovate aware of our schema.
package.json
Outdated
@@ -30,7 +30,8 @@ | |||
"test": "node Makefile.js test", | |||
"test:cli": "mocha", | |||
"test:fuzz": "node Makefile.js fuzz", | |||
"test:performance": "node Makefile.js perf" | |||
"test:performance": "node Makefile.js perf", | |||
"trunk": "trunk" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we'd probably want "lint": "trunk"
if the intent is to replace the current linting task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah - I just need to extract the json file check and then we're in business
@nzakas - I think this cleans up all the issues and at least let's you kick tires completely. I didn't see an active GitHub action that ran ESLint on itself - is the lint step right now fully optional? I can add a |
Because you're a first-time contributor, the lint check doesn't run automatically -- I just manually approved it so it will run now. It's titled "Verify Files" in the actions list. |
d
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it was included accidentally?
.trunk/trunk.yaml
Outdated
enabled: | ||
- eslint | ||
- actionlint@1.6.26 | ||
- checkov@3.1.51 | ||
- git-diff-check | ||
- markdownlint@0.38.0 | ||
- oxipng@9.0.0 | ||
- prettier@3.1.1 | ||
- renovate@37.122.0 | ||
- shellcheck@0.9.0 | ||
- shfmt@3.6.0 | ||
- svgo@3.2.0 | ||
- trivy@0.48.2 | ||
- trufflehog@3.63.7 | ||
- yamllint@1.33.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these versions correspond to the versions in package.json
? I'm just concerned about needing to update two places when these change.
...or do these have no relation to package.json
at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have no relation to package.json - I don't think you had/have versions of these tools running there. Some of these tools are nice to have - svgo, oxipng, shfmt, yamllint - nice tools to keep things clean and tidy.
Did you see any tool overlap - agreed - we don't want to have any collision with package.json
d
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Geeze - that's a ridiculous file.
I've tried checking this out locally and on my Windows machine I'm getting the following:
I tried running |
The product does work on windows - but not sure we've tested the npm flow - I am not a node/windows expert - but we have those on staff. They're taking a look. |
two issues here:
|
I'm using Git Bash. |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
.trunk/configs/.yamllint.yaml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There also exists an eslint-plugin-yml
. I suspect it would be good community-dogfooding for ESLint to use that rather than a separate tool. Is that something that can be reasonably done in metalinter without sacrificing features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take this as an action item for a follow up to use the eslint yaml plugin.
@EliSchleifer looks like we have the CI failing again on this. |
on it - also looking into how we can better support the mode of running on docs only in CI and not locally. small product change to make that a better experience and map to what we're aiming for here
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
hi @EliSchleifer, you can share the updates regarding this PR or if it is ready to review. |
I need to update the config - so the verification step is properly modeling the desire to handle CI flow differently from PR local flow. I should be able to update it this week |
.github/workflows/ci.yml
Outdated
@@ -33,9 +33,6 @@ jobs: | |||
- name: Check Licenses | |||
run: node Makefile checkLicenses | |||
|
|||
- name: Lint Docs JS Files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted this a separate step so we could more easily tell where a failure was on a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GitHub action from trunk will annotate the files inline with the issue. Is that not sufficient to get an understanding of the source of the problem.
I can create a sub branch to demonstrate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will those show up on the Discussion tab of the PR? What I'm looking for is the "glance-ability" of a PR so I don't have to switch to the code tab to get a general understanding which CI checks failed. This is key when we are barraged with PRs and trying to triage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @EliSchleifer
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Got a plan to make this work beautifully. Will post update tomorrow morning
________________________________
From: Joel Mathew Koshy ***@***.***>
Sent: Wednesday, March 13, 2024 7:19:07 PM
To: eslint/eslint ***@***.***>
Cc: Eli Schleifer ***@***.***>; Mention ***@***.***>
Subject: Re: [eslint/eslint] chore: migrate linting workflow to use trunk check metalinter (PR #17876)
@Rec0iL99 commented on this pull request.
________________________________
In .github/workflows/ci.yml<#17876 (comment)>:
@@ -33,9 +33,6 @@ jobs:
- name: Check Licenses
run: node Makefile checkLicenses
- - name: Lint Docs JS Files
ping @EliSchleifer<https://github.com/EliSchleifer>
—
Reply to this email directly, view it on GitHub<#17876 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAJVCPQ3LQQPFRPJPRPQOZ3YYECJXAVCNFSM6AAAAABA2WKCSOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMZVGUZDSMJVGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@EliSchleifer there are merge conflicts. Can you take a look? |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x ] Other, please explain: chore to migrate linting solution to trunk check - universal metalinter
What changes did you make? (Give an overview)
Trunk is a universal meta-linter and therefore obfuscates the need for separate calls to markdownlint / eslint / etc...
I've left in place the current git-hook implementation. I want to make sure the basic ergonomics work before migrating these scripts into trunk actions as well.
Is there anything you'd like reviewers to focus on?
Make sure you like the ergonomics of the tool as currently integrated