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

bump to maven 3.8.1 #148

Merged
merged 3 commits into from
May 13, 2021
Merged

bump to maven 3.8.1 #148

merged 3 commits into from
May 13, 2021

Conversation

car-roll
Copy link
Contributor

@car-roll car-roll commented May 12, 2021

Maven version 3.8.1 has some important security fixes that should be rolled into this plugin.

ping @centic9

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master 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

pom.xml Outdated
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>1.7.30</version>
Copy link
Member

Choose a reason for hiding this comment

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

should not be set here either - this is provided by jenkins and likely an older version.
if you use the version from the bom (set by the parent) at least your tests will blow up as oppsoed to tests passing then the plugin blowing up at runtime.

Whilst the plugin is using the pluginFirstClassloader the slf4j implementation coming from core will be a different (and older version) likely causing issues (if this is bundled, and if not - best not pretent that you have a version different to what you get)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds like the same logic would be applied to the guava dependency I had to add as well. The tests were failing because of the guice bump and I could only get it to pass by bumping guava (to at least 25.1-android)

Copy link
Member

@jtnord jtnord May 12, 2021

Choose a reason for hiding this comment

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

so guava is probably a bit different as it is not split to an api and implementation like slf4j is .

@car-roll car-roll requested a review from jtnord May 12, 2021 18:36
@centic9 centic9 merged commit 63fc02f into jenkinsci:master May 13, 2021
@jtnord
Copy link
Member

jtnord commented Jun 1, 2021

potentially caused a regression due to updated bundled Guice?
https://groups.google.com/g/jenkinsci-users/c/M7O4vOTE0Q8/m/5ucpEQeeCAAJ

@jglick
Copy link
Member

jglick commented Jun 1, 2021

If you are using pluginFirstClassLoader you ought to write a basic smoke test using RealJenkinsRule.

@MarkEWaite
Copy link
Contributor

See https://issues.jenkins.io/browse/JENKINS-65757 for another report of a regression due to updated bundled Guice.

@jtnord
Copy link
Member

jtnord commented Jun 8, 2021

see #151 for the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants