-
Notifications
You must be signed in to change notification settings - Fork 507
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
Help with eslint upgrade #2272
Comments
Signed-off-by: Baalekshan <[email protected]>
Signed-off-by: Baalekshan <[email protected]>
Signed-off-by: Baalekshan <[email protected]>
Signed-off-by: Baalekshan <[email protected]>
Signed-off-by: Baalekshan <[email protected]>
Signed-off-by: Baalekshan <[email protected]>
Hi @yurishkuro , unit-tests workflow is failing in my commit, how can i resolve |
## Which problem is this PR solving? #2272 ## Description of the changes - ## How was this change tested? - ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Baalekshan <[email protected]>
@Baalekshan the lint step is still failing after upgrade with
|
@yurishkuro Is this a concern ? Since, it was safely merged without any errors in the workflow. I didn't find this error in the PR raised only faced this in my forked repo. |
@Baalekshan the change we merged was good, but it did not resolve this ticket, because the upgrade is still failing, just on a different error. |
Oh right, what could be done now. Should I try fixing these errors? If so what can I do. |
The upgrade is trying to bump eslint from 8.x to 9.x, there could be some breaking changes causing this issue. |
I will look into it |
ESlint 9.x won't find There are two difference ways to resolve this.
|
Unfortunately, there are still some packages that can't support flat config (eslint.config.js)
It may be necessary to re-filter the |
So do you mean we can't migrate completely but also use .eslint.config.js partially? |
@Baalekshan Set the environment variable We can follow what @yurishkuro decides to do. |
@tico88612 Alright! |
@yurishkuro what should we do ? |
In the linked airbnb ticket there is some mentioning of compat workaround, maybe it could work. Otherwise we can wait till they support it. I don't see much point in using env var solution, if's probably temporary anyway. |
Has anyone figured out the issue? |
@yurishkuro above issue is solved or something is still wrong? Should I work on it. |
@MiirzaBaig @ayushrakesh There is a workaround but it's temporary. So, @yurishkuro suggested to wait until they support it. |
At this point I would go with a workaround, because airbnb ticket says it will take a while for them to fix due to dependencies. |
ok 👍 |
#2271 is failing to bump eslint versions:
.github/workflow/**
and.nvmrc
. It would be nice to enforce it via package.json.ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files
[eslint ] Error: Could not find config file.
The text was updated successfully, but these errors were encountered: