Skip to content

Conversation

timja
Copy link
Member

@timja timja commented Sep 23, 2024

see jenkinsci/plugin-pom#874

Depends on jenkins-infra/pipeline-library#883 pipeline library to add:
https://github.com/jenkinsci/jenkins/blob/master/Jenkinsfile#L175C33-L179

and jenkinsci/plugin-pom#1016

Testing done

Introduced lint warnings locally with both CI and not CI.
Tested with PR of pipeline library showing that reporting is correct
Tested with SNAPSHOT of parent pom showing that new opt-in method works:

# -Dtest for a faster build, it still runs the frontend tests
mvn test -Dtest=org.jenkins_ci.plugins.workflow.workflow_cps.InjectedTest -Dmaven.test.failure.ignore

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@timja timja requested a review from a team as a code owner September 23, 2024 15:07
@timja timja marked this pull request as draft September 23, 2024 15:07
plugin/pom.xml Outdated
Comment on lines 49 to 50
<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.

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.

@timja
Copy link
Member Author

timja commented Oct 3, 2024

@jglick any thoughts on bumping to parent pom 5.0? Requires updating the minimum jenkins.version to 2.479.

@timja timja added the chore label Oct 3, 2024
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Out of date. Sorry, do not recall seeing this before forgot this existed.

"babel-loader": "^9.0.1",
"clean-webpack-plugin": "^4.0.0",
"css-loader": "^5.0.0",
"eslint": "^8.41.0",
Copy link
Member

Choose a reason for hiding this comment

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

FYI #1014 (comment) beyond my skill level.

@timja timja force-pushed the mvn-eslint-fail branch from 054a167 to 5851831 Compare April 10, 2025 15:58
@timja timja marked this pull request as ready for review April 10, 2025 16:01
@timja timja requested review from a team and jglick April 10, 2025 16:01
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

#935 (comment) unless I am missing something

@timja timja requested a review from jglick April 10, 2025 18:46
@jglick
Copy link
Member

jglick commented Apr 10, 2025

Tested with PR of pipeline library showing that reporting is correct

To be clear, the effect of this PR would be that

  • before: lint warning ⇒ FAILURE, have to look at build log to find it
  • after: lint warning ⇒ UNSTABLE, maybe shown in Files, still blocks automatic merge

right?

@timja
Copy link
Member Author

timja commented Apr 10, 2025

Tested with PR of pipeline library showing that reporting is correct

To be clear, the effect of this PR would be that

  • before: lint warning ⇒ FAILURE, have to look at build log to find it
  • after: lint warning ⇒ UNSTABLE, maybe shown in Files, still blocks automatic merge

right?

correct.

image image image

This reverts commit b971b8e.
@jglick jglick enabled auto-merge (squash) April 10, 2025 20:00
@jglick jglick merged commit 2ef3c76 into jenkinsci:master Apr 10, 2025
17 checks passed
@timja timja deleted the mvn-eslint-fail branch April 11, 2025 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants