Skip to content
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

NO-ISSUE: Maven plugins update #2819

Closed
wants to merge 9 commits into from
Closed

Conversation

yesamer
Copy link
Contributor

@yesamer yesamer commented Dec 17, 2024

In this PR:

  • Updated Apache parent pom to version 33
  • Removed all plugin declarations already present in the Apache parent pom
  • Updated most of the maven plugin versions
  • Removed stale profile reference in serverless-workflow

@yesamer yesamer requested a review from tiagobento as a code owner December 17, 2024 10:04
@jomarko jomarko self-requested a review December 17, 2024 10:48
@yesamer yesamer added the area:dependencies Pull requests that update a dependency file label Dec 17, 2024
@tiagobento
Copy link
Contributor

@yesamer I guess I didn't write this anywhere, but the idea of managing the versions of Maven plugins ourselves is to have more control over patches that may contain security fixes. If we delegate it to the Apache parent we may find ourselves in a position of having to declare them anyway to fix something that the Apache parent hasn't.

@yesamer
Copy link
Contributor Author

yesamer commented Dec 17, 2024

@tiagobento Well, at the moment a CVE is detected in any plugin version, we can temporarily override. I guess that is a good compromise on having the same plugin versions used anywhere and keeping them updated.

Copy link
Contributor

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, my comments ale inline.

<version.maven.deploy.plugin>3.1.2</version.maven.deploy.plugin>
<version.maven.failsafe.plugin>${version.maven.surefire.plugin}</version.maven.failsafe.plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, this sounds a little bit non-standard to me, is that intentional to combine failsafe and surefire versions? furthermore version.maven.failsafe.plugin seems to be unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jomarko Yes, that's correct. failsafe and surefire have the same release cycle. apache pom manages that in the same way: https://repo.maven.apache.org/maven2/org/apache/apache/32/apache-32.pom

packages/dev-deployment-kogito-quarkus-blank-app/pom.xml Outdated Show resolved Hide resolved
packages/dev-deployment-kogito-quarkus-blank-app/pom.xml Outdated Show resolved Hide resolved
packages/dev-deployment-kogito-quarkus-blank-app/pom.xml Outdated Show resolved Hide resolved
<version.dependency.plugin>3.3.0</version.dependency.plugin>
<version.deploy.plugin>2.8.2</version.deploy.plugin>
<version.enforcer.plugin>3.1.0</version.enforcer.plugin>
<version.checkstyle.plugin>3.4.0</version.checkstyle.plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

is expected dashbuilder overrides to 3.1.1? I think preferred would be single version everywhere if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I purposely didn't touch dashbuilder because is not a project under our control, however, I can do a tentative

Copy link
Contributor Author

@yesamer yesamer Dec 18, 2024

Choose a reason for hiding this comment

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

@jomarko Updated versions in dashbuilder as well, thank you!

<version.deploy.plugin>2.8.2</version.deploy.plugin>
<version.enforcer.plugin>3.1.0</version.enforcer.plugin>
<version.checkstyle.plugin>3.4.0</version.checkstyle.plugin>
<version.clean.plugin>3.4.0</version.clean.plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

<version.enforcer.plugin>3.1.0</version.enforcer.plugin>
<version.checkstyle.plugin>3.4.0</version.checkstyle.plugin>
<version.clean.plugin>3.4.0</version.clean.plugin>
<version.compiler.plugin>3.13.0</version.compiler.plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

<version.compiler.plugin>3.13.0</version.compiler.plugin>
<version.dependency.plugin>3.7.1</version.dependency.plugin>
<version.deploy.plugin>3.1.2</version.deploy.plugin>
<version.enforcer.plugin>3.5.0</version.enforcer.plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

<version.failsafe.plugin>${version.surefire.plugin}</version.failsafe.plugin>
<version.install.plugin>2.5.2</version.install.plugin>
<version.install.plugin>3.1.2</version.install.plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

Copy link
Contributor

Choose a reason for hiding this comment

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

version misalignment is also for:

  • version.maven.artifact.plugin
  • version.resources.plugin
  • version.site.plugin
  • version.surefire.plugin
  • version.war.plugin

@yesamer
Copy link
Contributor Author

yesamer commented Dec 18, 2024

@tiagobento I checked the CVE history of all declared plugins in Apache parent pom.
An example: maven-assembly-plugin
https://security.snyk.io/package/maven/org.apache.maven.plugins%3Amaven-assembly-plugin
https://mvnrepository.com/artifact/org.apache.maven.plugins/maven-assembly-plugin
I didn't find any reported CVE for those plugins EVER.
Based on that, I guess we are safe to proceed ;)

@tiagobento
Copy link
Contributor

@yesamer Problems are often in transitive dependencies...

@yesamer
Copy link
Contributor Author

yesamer commented Dec 18, 2024

@tiagobento Even in such a case, those dependencies are not transitively imported in the production code, as the dependencies imported by the plugin are used during its own compile time only.

Copy link
Contributor

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

Thank you. I did a small sanity check with the online-editor and extended-services:

  • open bpmn sample
  • open dmn sample, old dmn gwt editor
  • run dmn model, old dmn gwt editor

Copy link
Contributor

@tiagobento tiagobento left a comment

Choose a reason for hiding this comment

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

I don't see it that simply @yesamer... I'm not 100% onboard with this version management strategy this PR introduces, as it delegates to Apache parent for maven-base but not for some other packages. I'd like to hold merging it so we can discuss it in more depth. Such a change is not to be done casually, IMHO, as it has to do with the way we manage and maintain dependencies in general.

@tiagobento tiagobento added pr: DO NOT MERGE Draft PR, not ready for merging area:monorepo labels Dec 20, 2024
@yesamer
Copy link
Contributor Author

yesamer commented Dec 21, 2024

@tiagobento Of course, I'm available to discuss this topic in private, but if you think for real I did this PR casually after all the evidences I gave to you, trying to explain that the plugin management is NOT THE SAME of the dependency management, I think I failed my intent, and I no more interested to keep this effort in place.

@yesamer yesamer closed this Dec 21, 2024
@yesamer
Copy link
Contributor Author

yesamer commented Dec 22, 2024

@tiagobento Many apologies for my impulsive rant, that's not my standard and my style. Taking a real break here, we can talk about this in private after the vacation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dependencies Pull requests that update a dependency file area:monorepo pr: DO NOT MERGE Draft PR, not ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants