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

Unexpected addition of commons-text-api test dependency on BOM upgrade #2484

Closed
MarkEWaite opened this issue Sep 10, 2023 · 5 comments
Closed
Labels
bug Something isn't working

Comments

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Sep 10, 2023

Jenkins and plugins versions report

Environment
Elastic axis plugin upgrade from plugin bill of materials from 2401.v7a_d68f8d0b_09 to 2423.vce598171d115

What Operating System are you using (both controller, and any agents involved in the problem)?

Running Java 11, Java 17, or Java 21 on Linux

Reproduction steps

  1. Attempt to upgrade the plugin bill of materials from 2401.v7a_d68f8d0b_09 to 2423.vce598171d115
  2. Detect the InjectedTest failure because plugin-utils-api cannot load without commons-text-api and commons-lang3-api
  3. Insert a test dependency on commons-text-api plugin and the InjectedTest passes

Expected Results

I assumed that this bom upgrade would be like previous bom upgrades, with no changes beyond the update of the bom version number. That was the behavior for many of the other plugins that I maintain, but not for the elastic axis plugin.

Actual Results

I was surprised that a new test dependency was required in the elastic axis plugin for this bom upgrade when it has not been required in previous bom upgrades.

Anything else?

I'd like to understand the difference between previous bom releases and the most recent bom release. The most recent bom release upgraded the commons-text-api plugin but an earlier upgrade to that plugin was processed a month ago without requiring the addition of a new test dependency.

I'm also happy if the answer is "this is not a surprise" for others.

@MarkEWaite MarkEWaite added the bug Something isn't working label Sep 10, 2023
@basil
Copy link
Member

basil commented Sep 11, 2023

checks-api 2.0.1 contained a plugin parent POM upgrade that (erroneously) removed its dependencies on commons-text-api and commons-lang3-api from the flattened POM (but not MANIFEST.MF, so test-only breakage rather than a production issue). The change in behavior was apparently caused by mojohaus/flatten-maven-plugin#329, released in 1.4.0 and adopted in plugin parent POM 4.57 in March 2023 but apparently not by Dr Hafner until September 2023. checks-api has a nonstandard parent POM that includes the dependencies on commons-text-api and commons-lang3-api, which was working only by accident prior to mojohaus/flatten-maven-plugin#329 and does not work at all after mojohaus/flatten-maven-plugin#329 due to plugin-pom's use of the default value for https://www.mojohaus.org/flatten-maven-plugin/flatten-mojo.html#flattenDependencyMode. It would be fair to characterize this usage as "unsupported." I recommend deleting the commons-text-api and commons-lang3-api dependencies from analysis-pom and inlining them into each consumer.

@jonesbusy
Copy link
Contributor

@basil
Copy link
Member

basil commented Sep 12, 2023

Since this is a problem with jenkinsci/analysis-pom-plugin and/or jenkinsci/checks-api-plugin rather than jenkinsci/bom, I am closing this ticket. BOM maintainers are encouraged to revert the problematic change to restore stability to BOM if necessary. Consumers of the BOM are asked to express themselves in jenkinsci/checks-api-plugin#233 rather than in this ticket.

@basil basil closed this as not planned Won't fix, can't repro, duplicate, stale Sep 12, 2023
MarkEWaite added a commit to MarkEWaite/bom that referenced this issue Sep 16, 2023
…enkinsci#2471)"

jenkinsci#2484 describes the unexpected
addition of commons-text-api as a new dependency for many consumers of
the plugin bill of materials.

Rather than having more and more plugins adding dependencies on the
commons-text-api or the commons-lang3-api plugin, let's keep the
checks-api dependency at 2.0.0 instead of 2.0.1.

jenkinsci/checks-api-plugin#233 is the issue
reported to the checks-api plugin.  Once that issue is resolved, we
should be able to use more recent checks-api plugin versions.

Dependency updates that had to add commons-text-api included:

* jenkinsci/bitbucket-kubernetes-credentials-plugin#133
* jenkinsci/elastic-axis-plugin#309
* jenkinsci/nodelabelparameter-plugin#265
* jenkinsci/testng-plugin-plugin#244

This reverts commit 729dfb2.
MarkEWaite added a commit to MarkEWaite/checks-api-plugin that referenced this issue Sep 16, 2023
Fixes jenkinsci#233 (at least partially)

The plugin depends on Apache commons lang3 but did not declare it
directly in the pom.

Declare it in the pom with hope that will help to resolve the plugin
bill of materials issue described at
jenkinsci/bom#2484 (comment)
MarkEWaite added a commit to MarkEWaite/bom that referenced this issue Sep 17, 2023
…enkinsci#2471)"

jenkinsci#2484 describes the unexpected
addition of commons-text-api as a new dependency for many consumers of
the plugin bill of materials.

Rather than having more and more plugins adding dependencies on the
commons-text-api or the commons-lang3-api plugin, let's keep the
checks-api dependency at 2.0.0 instead of 2.0.1.

jenkinsci/checks-api-plugin#233 is the issue
reported to the checks-api plugin.  Once that issue is resolved, we
should be able to use more recent checks-api plugin versions.

Dependency updates that had to add commons-text-api included:

* jenkinsci/bitbucket-kubernetes-credentials-plugin#133
* jenkinsci/elastic-axis-plugin#309
* jenkinsci/nodelabelparameter-plugin#265
* jenkinsci/testng-plugin-plugin#244

This reverts commit 729dfb2.
MarkEWaite added a commit that referenced this issue Sep 17, 2023
* Pin previous credentials-binding release for LTS profiles

The most recent release of the credentials-binding plugin adds masking
for base64 credentials.  That's a nice improvement.  Unfortunately,
it causes one of the config-file-provider tests to fail.

Adapt older bom profiles to "Bump credentials-binding (#2509)" by
retaining the current version of the credentials binding plugin on
the weekly release and pinning the previous credentials binding plugin
release on the LTS releases.

Could exclude the test failure on all releases, but it seemed better
to be able to detect test failures from the weekly release even if we
can't yet test the new version with the LTS releases.

This partially reverts commit bab8257.

* Revert "Bump checks-api.version from 2.0.0 to 2.0.1 in /bom-weekly (#2471)"

#2484 describes the unexpected
addition of commons-text-api as a new dependency for many consumers of
the plugin bill of materials.

Rather than having more and more plugins adding dependencies on the
commons-text-api or the commons-lang3-api plugin, let's keep the
checks-api dependency at 2.0.0 instead of 2.0.1.

jenkinsci/checks-api-plugin#233 is the issue
reported to the checks-api plugin.  Once that issue is resolved, we
should be able to use more recent checks-api plugin versions.

Dependency updates that had to add commons-text-api included:

* jenkinsci/bitbucket-kubernetes-credentials-plugin#133
* jenkinsci/elastic-axis-plugin#309
* jenkinsci/nodelabelparameter-plugin#265
* jenkinsci/testng-plugin-plugin#244

This reverts commit 729dfb2.
@uhafner
Copy link
Member

uhafner commented Sep 19, 2023

Since this is a problem with jenkinsci/analysis-pom-plugin and/or jenkinsci/checks-api-plugin rather than jenkinsci/bom, I am closing this ticket. BOM maintainers are encouraged to revert the problematic change to restore stability to BOM if necessary. Consumers of the BOM are asked to express themselves in jenkinsci/checks-api-plugin#233 rather than in this ticket.

I think that I do not understand your conclusion, can you please elaborate. Defining dependencies in a parent pom is a key concept of maven. So if this concept does not work in our build chain I would assume that our build chain has a defect that needs to be fixed.

@basil
Copy link
Member

basil commented Sep 19, 2023

I would assume that our build chain has a defect that needs to be fixed.

Not when you are the only one in the Jenkins plugin community who is doing this and when the Maven developers have explicitly changed this behavior (including adding a test for the new behavior). Your choices are to take this up with the Maven developers or to conform to the rest of the Jenkins plugin community by removing plugin-to-plugin dependencies from analysis-pom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants