build(eslint): Ignore Symlinks and Reduce Violation Limit #36010
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kdmccormick
force-pushed
the
kdmccormick/eslint-fix
branch
from
81601c5 to
75d0293
Compare
kdmccormick
changed the title
|
FYI @salman2013 -- another existing issue with the CI 😛 |
Background: We have a large number of standing eslint violations in our legacy frontends. Rather than fixing or amnesty-ing all of them, we've opted to use a simple integer limit of violations. Exceeding this limit causes the quality check to fail. Before we moved eslint off of Paver [1], the limit was unreasonably high (probably due to deprecations that removed violation-rich frontend code). This was good, except for the fact that we essentially weren't catching when new violations creeped into the JS code. So, in [1], we lowered the limit down to the lowest possible value, which we thought was 1285. However, we've found that this made the check flaky-- turned out, we have been unintentionally double-counting various violations due to the symlinks in edx-platform. Some of those symlinks' existence is dependent on whether and how `npm ci` and `npm run build` have been run. As a result, 1285 would pass in some contexts, and fail in other contexts. The fix is to simply add all the relevant edx-platform symlinks to .eslintignore. This allows us to lower the violations limit to 734. [1] openedx#35159
kdmccormick
force-pushed
the
kdmccormick/eslint-fix
branch
from
75d0293 to
61a8fa6
Compare
robrap
approved these changes
Dec 11, 2024
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Background: We have a large number of standing eslint violations in our
legacy frontends. Rather than fixing or amnesty-ing all of them, we've
opted to use a simple integer limit of violations. Exceeding this
limit causes the quality check to fail.
Before we moved eslint off of Paver [1], the limit was unreasonably
high (probably due to deprecations that removed violation-rich frontend
code). This was good, except for the fact that we essentially weren't
catching when new violations creeped into the JS code.
So, in [1], we lowered the limit down to the lowest possible value,
which we thought was 1285. However, we've found that this made the check
flaky-- turned out, we have been unintentionally double-counting various
violations due to the symlinks in edx-platform. Some of those symlinks'
existence is dependent on whether and how
npm ciandnpm run buildhave been run. As a result, 1285 would pass in some contexts, and fail
in other contexts.
The fix is to simply add all the relevant edx-platform symlinks to
.eslintignore. This allows us to lower the violations limit to 734.
[1] #35159
Testing
Looking at the Quality Others CI logs, we see that eslint passes with a number of violations equal to the new limit (734):
Merge deadline
Asap, to unblock master build