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: Upgrade Maven plugins to newer versions and Apache parent from 32 to 33 #2819

Merged
merged 22 commits into from
Jan 16, 2025

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.

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
packages/stunner-editors/pom.xml Show resolved Hide resolved
packages/stunner-editors/pom.xml Show resolved Hide resolved
packages/stunner-editors/pom.xml Show resolved Hide resolved
packages/stunner-editors/pom.xml Show resolved Hide resolved
packages/stunner-editors/pom.xml Outdated Show resolved Hide resolved
@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 yesamer closed this Dec 21, 2024
@yesamer yesamer reopened this Jan 7, 2025
@yesamer yesamer added the pr: wip PR is still under development label Jan 7, 2025
@yesamer
Copy link
Contributor Author

yesamer commented Jan 7, 2025

@tiagobento Setting this PR on hold until you share your input on the whole strategy. Again, apologies for my previous bad answer that I removed.

@yesamer
Copy link
Contributor Author

yesamer commented Jan 14, 2025

@tiagobento After our discussion now I fully understand the scenario you described, and I agree it can be a realistic scenario. Thank you for raising that and apologize again for not giving the proper attention.
Now, to finalize this PR, I'm open to considering your suggestion and don't rely on the Apache parent pom for the involved maven plugins, to quickly react to the worst-case scenario of overriding a transitive dependency used by a maven plugin.

@yesamer yesamer removed the pr: wip PR is still under development label Jan 14, 2025
<artifactId>maven-helper-plugin</artifactId>
<version>${version.maven.helper.plugin}</version>
<artifactId>maven-help-plugin</artifactId>
<version>${version.maven.help.plugin}</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maven-helper-plugin doesn't exist

@tiagobento tiagobento changed the title NO-ISSUE: Maven plugins update NO-ISSUE: Upgrade Maven plugins to newer versions and Apache parent from 32 to 33 Jan 16, 2025
@tiagobento tiagobento merged commit f7beeed into apache:main Jan 16, 2025
15 checks passed
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.

4 participants