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

Declare config file provider test dependency #133

Closed
wants to merge 1 commit into from

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Dec 13, 2022

Declare config file provider test dependency

Tests run successfully without this dependency when executed from the usual Apache Maven command line as in:

$ mvn clean verify

Tests fail when run in the BOM evaluation as in jenkinsci/bom#1633 . They report that a class from the config file provider plugin could not be found.

jenkinsci/bom#1633 (comment) provides details.

Testing performed

Duplicated the problem by checking out jenkinsci/bom#1633 and compiling the megawar in the bom directory with:

PLUGINS=matrix-auth TEST=InjectedTest bash local-test.sh

Then built the matrix-auth plugin with this command line

BOM_DIR=/home/mwaite/hub/core/bom
mvn -Denforcer.skip=true -Djenkins.version=2.381 -Djth.jenkins-war.path=$BOM_DIR/target/local-test/megawar.war \
-DoverrideWar=$BOM_DIR/target/local-test/megawar.war -DoverrideWarAdditions=true -DuseUpperBounds=true \
-DupperBoundsExcludes=javax.servlet:servlet-api \
-Dtest=ImportTest \
clean verify

That shows the failure in the matrix-auth 3.1.6 release.

Then modified the matrix-auth pom.xml file as noted in this commit and ran the same code again. The test passed.

Rationale

The additional test dependency does not modify the production code. It will allow the next release of the matrix-auth plugin to be included in the Jenkins plugin bill of materials.

  • 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

Tests run successfully without this dependency when executed from the
usual Apache Maven command line as in:

$ `mvn clean verify`

Tests fail when run in the BOM evaluation as in
jenkinsci/bom#1633 .  They report that a class
from the config file provider plugin could not be found.

jenkinsci/bom#1633 (comment)
provides details.

Testing performed:

Duplicated the problem by checking out
jenkinsci/bom#1633 and compiling the megawar in
the bom directory with:

```
PLUGINS=matrix-auth TEST=InjectedTest bash local-test.sh
```

Then built the matrix-auth plugin with this command line

```
BOM_DIR=/home/mwaite/hub/core/bom
mvn -Denforcer.skip=true -Djenkins.version=2.381 \
-Djth.jenkins-war.path=$BOM_DIR/target/local-test/megawar.war \
-DoverrideWar=$BOM_DIR/target/local-test/megawar.war \
-DoverrideWarAdditions=true -DuseUpperBounds=true \
-DupperBoundsExcludes=javax.servlet:servlet-api \
-Dtest=ImportTest \
clean verify
```

That shows the failure in the matrix-auth 3.1.6 release.

Then modified the matrix-auth pom.xml file as noted in this commit and
ran the same code again.  The test passed.

The additional test dependency does not modify the production code.
It will allow the next release of the matrix-auth plugin to be included
in the Jenkins plugin bill of materials.
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.

@daniel-beck please release

@daniel-beck
Copy link
Member

Why is the fix needed in this plugin and not in whatever upstream component changed? jenkinsci/bom#1435 (comment) looks like an explanation but I don't get it. Wouldn't this apply to all dependencies?

Also, nothing about the job-dsl dependency changed. configuration-as-code was updated though…?

Not opposed to merging, but I'd like to understand it first given this doesn't look too urgent. (If it is, an explanation why would be nice.)

@MarkEWaite
Copy link
Contributor Author

Not opposed to merging, but I'd like to understand it first given this doesn't look too urgent. (If it is, an explanation why would be nice.)

I think that the merge is more an effort to keep versions current in the plugin bom than it is anything urgent. I don't think there is any particular urgency to merge it, though not merging it may cause other surprises, since matrix-auth 3.1.6 then won't be tested as part of the plugin bom test suites.

The plugin bill of materials will continue to deliver matrix-auth 3.1.5 rather than 3.1.6, but since I've closed the pull request that proposed to update to matrix-auth 3.1.6, maintainers of the plugin bom won't see new pull requests for matrix-auth plugin until the next release of matrix-auth plugin. I don't object to leaving the matrix-auth dependency at 3.1.5 in the plugin bom while more investigation is done. I'm not personally willing to do that further investigation because I think it is simpler to resolve the issue by adding a test dependency than by doing the root cause analysis to identify which specific change introduced the issue. Apologies for my laziness in this case, but a new test dependency seems much easier than bisecting the changes to the matrix-auth plugin to find an upstream root cause.

@basil
Copy link
Member

basil commented Mar 28, 2023

Just ran into this issue yet again in another BOM PR. It has been 3 months and 15 days since this PR has been filed; what gives?

@basil
Copy link
Member

basil commented Apr 17, 2023

Just ran into this issue yet again in another BOM PR. It has been 3 months and 15 days since this PR has been filed; what gives?

I asked the above question 3 weeks ago. @daniel-beck did not respond.

@daniel-beck
Copy link
Member

daniel-beck commented Apr 17, 2023

@daniel-beck did not respond

For a narrow definition of not responding, sure. #136 and jenkinsci/bom#1911 are a response to the underlying problem.

At the moment, I'm looking to integrate changes to justify a release with #136. While I made the nicer DSL in #111 (comment) work, I haven't figured out how to adapt it so the Job DSL CasC looks better (and IMO it doesn't make sense to integrate the YAML DSL change in isolation). Right now, I'm looking at JENKINS-67368 to at least have something.

@basil
Copy link
Member

basil commented Apr 17, 2023

At the moment, I'm looking to integrate changes to justify a release with #136.

Am I understanding correctly that unblocking others in BOM did not justify a release?

@daniel-beck
Copy link
Member

unblocking others in BOM did not justify a release

I wouldn't want to block others work, but so far I am unaware that I am blocking anything. AFAIUI, the problem was that matrix-auth couldn't have its version increased in the BOM beyond the previous minor version. At the same time, matrix-auth-3.1.5...matrix-auth-3.1.6 has only fairly irrelevant changes and #136 demonstrates a lack of real PCT failures, indicating applying the update isn't particularly urgent. So it is unclear to me what this is blocking, beyond a general intention to track latest releases (and in this case, only a single release–the change addressing the problem is going into whatever the next one is). Based on that information, this didn't seem to justify a new update notification to every Jenkins admin for a no-op release. If I am mistaken, please explain.

@basil
Copy link
Member

basil commented Apr 17, 2023

As I wrote very clearly in …

Just ran into this issue yet again in another BOM PR.

… this was blocking me in jenkinsci/bom#1908, and I was only able to move forward in that PR by reducing test coverage (disabling org.jenkinsci.plugins.matrixauth.integrations.casc.ImportTest). That could have been avoided if you had merged and released this PR as requested.

@MarkEWaite MarkEWaite deleted the fix-bom-tests branch January 5, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants