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

Depend on commons-text-api #1979

Merged
merged 1 commit into from
Jun 4, 2022
Merged

Conversation

sghill
Copy link
Contributor

@sghill sghill commented Jun 2, 2022

Hi,

I have introduced commons-text-api, which in turn depends on commons-lang3-api. This PR replaces the dependency on the commons-text jar with the plugin, and now both jars are absent from the hpi:

$ zipinfo -1 plugin/target/configuration-as-code.hpi
META-INF/
META-INF/MANIFEST.MF
WEB-INF/
WEB-INF/lib/
img/
META-INF/maven/
META-INF/maven/io.jenkins/
META-INF/maven/io.jenkins/configuration-as-code/
WEB-INF/lib/vavr-match-0.10.4.jar
WEB-INF/lib/json-20211205.jar
WEB-INF/lib/configuration-as-code.jar
WEB-INF/lib/jsr305-3.0.2.jar
WEB-INF/lib/vavr-0.10.4.jar
WEB-INF/licenses.xml
img/logo-head.svg
img/logo.png
img/logo.eps
META-INF/maven/io.jenkins/configuration-as-code/pom.xml
META-INF/maven/io.jenkins/configuration-as-code/pom.properties

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull 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 in Jenkins JIRA
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Did you provide a test-case? That demonstrates feature works or fixes the issue. (existing tests still pass)

@sghill sghill requested a review from a team as a code owner June 2, 2022 04:12
@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #1979 (8c970ab) into master (bda8435) will increase coverage by 0.07%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1979      +/-   ##
============================================
+ Coverage     80.81%   80.89%   +0.07%     
+ Complexity      838      837       -1     
============================================
  Files            71       71              
  Lines          2471     2465       -6     
  Branches        346      346              
============================================
- Hits           1997     1994       -3     
+ Misses          365      362       -3     
  Partials        109      109              
Impacted Files Coverage Δ
...c/core/HudsonPrivateSecurityRealmConfigurator.java 96.15% <0.00%> (+4.77%) ⬆️

plugin/pom.xml Outdated
@@ -86,6 +86,19 @@
<groupId>org.apache.commons</groupId>
<artifactId>commons-text</artifactId>
<version>1.9</version>
<exclusions>
Copy link
Member

@timja timja Jun 2, 2022

Choose a reason for hiding this comment

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

I wonder if it’s better that commons text is packaged as one too as that’s where commons lang comes from in a lot of plugins

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. I've seen it as a dependency on other plugins.

Is there a quick way to tell how many plugins bundle commons-text today?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jetersen! The number is potentially quite a bit higher, because it could be brought in transitively I think?

20 sounds like a good reason for an api plugin, but I'm not sure if there are already rules for when to introduce an api plugin.

Copy link
Member

Choose a reason for hiding this comment

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

normally 2 or more is enough to justify an api plugin.

Copy link
Member

@jetersen jetersen Jun 3, 2022

Choose a reason for hiding this comment

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

Copy link
Member

@jetersen jetersen Jun 3, 2022

Choose a reason for hiding this comment

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

@sghill I'd suggest keeping changelog out of the README and use GitHub releases as they integrate with Jenkins Plugin site.

See: https://plugins.jenkins.io/configuration-as-code/#releases

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks very much for the help (and patience!) @jetersen. This is now published and I have updated this PR to rely on commons-text-api, which in turn relies on commons-lang3-api.

Separately, I noticed commons-lang3-api has a nonstandard versioning scheme (3.12.0.0, rather than 3.12.0-v{count}.{sha}). Is that something we want to fix before rolling this out, or is it fine?

Copy link
Member

Choose a reason for hiding this comment

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

Can be fixed at any point

@sghill sghill changed the title Depend on commons-lang3-api plugin instead of commons-lang3 Depend on commons-text-api plugin instead of commons-text Jun 3, 2022
@jetersen jetersen changed the title Depend on commons-text-api plugin instead of commons-text Depend on commons-text-api Jun 4, 2022
@jetersen jetersen merged commit a27c48c into jenkinsci:master Jun 4, 2022
@jglick
Copy link
Member

jglick commented Jul 18, 2022

See jenkinsci/commons-lang3-api-plugin#18. If configuration-as-code is going to depend on this plugin then we need to make sure it is kept in good condition, which currently it is not; likely needs comaintainers with more Jenkins core experience.

@timja @jetersen perhaps this change could be reverted on a temporary basis to address the emergency?

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

Successfully merging this pull request may close these issues.

None yet

4 participants