Skip to content

Configure eslint for warnings-ng to report on CI #935

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Apr 10, 2025
Merged

Configure eslint for warnings-ng to report on CI #935

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a577798
Set yarn version this project uses
timja Sep 23, 2024
72392fd
Configure eslint for warnings-ng to report on CI
timja Sep 23, 2024
6dfae9c
See if this works on Windows
timja Sep 23, 2024
972f591
Add lint issue
timja Sep 23, 2024
d6f1363
Opt-in to new system in upcoming parent pom
timja Sep 23, 2024
81f1e6b
Revert change
timja Sep 23, 2024
5851831
Merge branch 'master' into mvn-eslint-fail
timja Apr 10, 2025
ecfb541
Fix fmt
timja Apr 10, 2025
7394379
Extra new line
timja Apr 10, 2025
6d00f1b
Use prefixed property
timja Apr 10, 2025
46a0a3d
Remove gitignore
timja Apr 10, 2025
b971b8e
Lint error
timja Apr 10, 2025
23593fe
Revert "Lint error"
timja Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions plugin/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.yarn/
7 changes: 5 additions & 2 deletions plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
"prod": "webpack --config webpack.prod.js",
"start": "yarn dev --watch",
"lint:js": "eslint src/main/js --ext js",
"lint:js-ci": "eslint src/main/js -f checkstyle -o target/eslint-warnings.xml --ext js",
"mvnbuild": "yarn prod",
"mvntest": "yarn lint:js"
"mvntest": "if-env CI=true && yarn lint:js-ci || yarn lint:js"
},
"repository": {
"type": "git",
Expand All @@ -22,6 +23,7 @@
"@babel/cli": "^7.27.0",
"@babel/core": "^7.26.10",
"@babel/preset-env": "^7.26.9",
"@delucis/if-env": "^1.1.2",
"autoprefixer": "^10.4.21",
"babel-loader": "^10.0.0",
"clean-webpack-plugin": "^4.0.0",
Expand All @@ -48,5 +50,6 @@
"browserslist": [
"defaults",
"not IE 11"
]
],
"packageManager": "[email protected]"
}
4 changes: 4 additions & 0 deletions plugin/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@
<no-test-jar>false</no-test-jar>
<node.version>18.20.0</node.version>
<yarn.version>1.22.19</yarn.version>

<!-- Opt in to Jenkins handling eslint results -->
<maven.test.failure.ignore>false</maven.test.failure.ignore>
<testFailureIgnore>${maven.test.failure.ignore}</testFailureIgnore>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in plugin-pom?

Safer to configure this in build/plugins/*/configuration than to rely on setting a generic-sounding property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the opting in to say that that we have configured test/lint properly for Jenkins build reporting. It is in plugin-pom here: https://github.com/jenkinsci/plugin-pom/pull/1016/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R149

We could prefix it, e.g. something like:

Suggested change
<maven.test.failure.ignore>false</maven.test.failure.ignore>
<testFailureIgnore>${maven.test.failure.ignore}</testFailureIgnore>
<maven.test.failure.ignore>false</maven.test.failure.ignore>
<frontend.testFailureIgnore>${maven.test.failure.ignore}</frontend.testFailureIgnore>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think prefixing it would be prudent. As written it looks like it is a magic property name interpreted by some unknown mojo. But https://github.com/eirslett/frontend-maven-plugin?tab=readme-ov-file#ignoring-failure seems to imply that regular maven.test.failure.ignore is honored? https://github.com/eirslett/frontend-maven-plugin/blob/4a501a10677716a0e6676b603e3b93bc6bf319f1/frontend-maven-plugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/AbstractFrontendMojo.java#L33-L40 so why do we even need to configure the plugin specially for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see jenkinsci/plugin-pom#1015 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this still does not make sense to me and the link did not clear it up. There should be a /build/plugins/plugin section for frontend-maven-plugin with configuration rather than using a magic property name which does not even mention frontend.

Current trunk prefixes it https://github.com/jenkinsci/plugin-pom/blob/c4f78bcc60b0fe92a86f9d971de2b5e05caa513b/README.md?plain=1#L197 https://github.com/jenkinsci/plugin-pom/blob/c4f78bcc60b0fe92a86f9d971de2b5e05caa513b/pom.xml#L170 https://github.com/jenkinsci/plugin-pom/blob/c4f78bcc60b0fe92a86f9d971de2b5e05caa513b/pom.xml#L1232

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC it was to make it opt-in and to allow us to control it in the plugin-pom at a later date to change the default i.e. remove the configuration.

It was opt-in because if a plugin picked up the change and hadn't configured it then lint issues would silently fail and not be reported, worse than the status quo.

</properties>
<dependencyManagement>
<dependencies>
Expand Down
19 changes: 19 additions & 0 deletions plugin/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,13 @@
"@babel/helper-string-parser" "^7.25.9"
"@babel/helper-validator-identifier" "^7.25.9"

"@delucis/if-env@^1.1.2":
version "1.1.2"
resolved "https://registry.yarnpkg.com/@delucis/if-env/-/if-env-1.1.2.tgz#5f315d9e706d24aacab76d035c9191eb396e3b90"
integrity sha512-sZJF62edo927tjq0cANW9Quozwmact7o4fIbPdvmTLqDxQgXJE9xqlkspvgGAZINL3JkwUFTQiGKAan/e2sDnQ==
dependencies:
matcher "^1.1.1"

"@discoveryjs/json-ext@^0.6.1":
version "0.6.3"
resolved "https://registry.yarnpkg.com/@discoveryjs/json-ext/-/json-ext-0.6.3.tgz#f13c7c205915eb91ae54c557f5e92bddd8be0e83"
Expand Down Expand Up @@ -1495,6 +1502,11 @@ escalade@^3.2.0:
resolved "https://registry.yarnpkg.com/escalade/-/escalade-3.2.0.tgz#011a3f69856ba189dffa7dc8fcce99d2a87903e5"
integrity sha512-WUj2qlxaQtO4g6Pq5c29GTcWGDyd8itL8zTlipgECz3JesAiiOKotd8JU6otB3PACgG6xkJUyVhboMS+bje/jA==

escape-string-regexp@^1.0.4:
version "1.0.5"
resolved "https://registry.yarnpkg.com/escape-string-regexp/-/escape-string-regexp-1.0.5.tgz#1b61c0562190a8dff6ae3bb2cf0200ca130b86d4"
integrity sha512-vbRorB5FUQWvla16U8R/qgaFIya2qGzwDrNmCZuYKrbdSUMG6I1ZCGQRefkRVhuOkIGVne7BQ35DSfo1qvJqFg==

escape-string-regexp@^4.0.0:
version "4.0.0"
resolved "https://registry.yarnpkg.com/escape-string-regexp/-/escape-string-regexp-4.0.0.tgz#14ba83a5d373e3d311e5afca29cf5bfad965bf34"
Expand Down Expand Up @@ -2076,6 +2088,13 @@ make-dir@^2.1.0:
pify "^4.0.1"
semver "^5.6.0"

matcher@^1.1.1:
version "1.1.1"
resolved "https://registry.yarnpkg.com/matcher/-/matcher-1.1.1.tgz#51d8301e138f840982b338b116bb0c09af62c1c2"
integrity sha512-+BmqxWIubKTRKNWx/ahnCkk3mG8m7OturVlqq6HiojGJTd5hVYbgZm6WzcYPCoB+KBT4Vd6R7WSRG2OADNaCjg==
dependencies:
escape-string-regexp "^1.0.4"

merge-stream@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/merge-stream/-/merge-stream-2.0.0.tgz#52823629a14dd00c9770fb6ad47dc6310f2c1f60"
Expand Down