Skip to content

Modernise pom#63

Merged
timja merged 9 commits into
jenkinsci:masterfrom
timja:modernise-pom
Feb 28, 2021
Merged

Modernise pom#63
timja merged 9 commits into
jenkinsci:masterfrom
timja:modernise-pom

Conversation

@timja

@timja timja commented Aug 8, 2020 •
edited by vgaidarji
Loading

Copy link
Copy Markdown
Member

What has been done

  1. Modernise pom by updating to newer versions of dependencies and required Jenkins version
    • Jenkins plugins 2.32 -> 4.7
    • Jenkins 2.0 -> 2.204.6
  2. Update required Java version 7 -> 8
  3. Use https://github.com/jenkinsci/bom to satisfy upper boundary dependencies and no longer manage explicit versions but rather rely on

How to test

  1. Make sure project compiles and CI is 🟢
  2. Perform the plugin update to new version packaged from current PR to smoke test plotting capabilities after migration to Java 8
  3. Make sure Jenkins log doesn't throw any new errors/warnings

Checklist

  • Git commits follow best practices
  • Build passes in Jenkins
  • Appropriate tests or explanation to why this change has no tests
  • Pull Request is marked with appropriate label (see .github/release-drafter.yml)
  • JIRA issue is well described (problem explanation, steps to reproduce, screenshots)
  • For dependency updates: links to external changelogs and, if possible, full diffs

@vgaidarji vgaidarji added dependencies Dependencies update chore General project maintenance and removed chore General project maintenance labels Sep 20, 2020
vgaidarji
vgaidarji reviewed Sep 20, 2020
View reviewed changes

@vgaidarji vgaidarji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes in the PR are good with me 👍
No need to update any classes yet to use Java 8 features.

How does this help with https://issues.jenkins-ci.org/browse/JENKINS-62800? Or it's just the initial project cleanup and the actual fix comes next?

Please provide before/after screenshots for the tables to div fix when ready.

Comment thread pom.xml
Comment thread pom.xml
Comment thread pom.xml
Comment thread pom.xml
Comment thread pom.xml
Comment thread pom.xml
Comment thread pom.xml
(xml-apis.jar has an old version of the DOM.
Eclipse tends to get the classpath order wrong)
-->
<plugin>

@vgaidarji vgaidarji Sep 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, please remove the comment above as well.

@vgaidarji

vgaidarji commented Sep 20, 2020

Copy link
Copy Markdown
Contributor

image
The build is failing due to Checkstyle warnings. It wasn't the case before and should be caused by upstream changes in buildPlugin()

Those TODO's should probably be removed.

@vgaidarji

vgaidarji commented Sep 20, 2020

Copy link
Copy Markdown
Contributor

Since buildPlugin() already works with JDK 8 by default, no changes required
image

@timja

timja commented Sep 20, 2020

Copy link
Copy Markdown
Member Author

This was just an initial PR to make testing the plugin easier against the new Jenkins version

timja
timja commented Sep 21, 2020
View reviewed changes
Comment thread pom.xml Outdated
Comment thread pom.xml
Comment thread pom.xml
@timja timja mentioned this pull request Sep 21, 2020
Merged
@vgaidarji

vgaidarji commented Sep 24, 2020

Copy link
Copy Markdown
Contributor

@timja thanks for updates here. Will be ok updating the current PR to be focused only on Java 8 migration and adjustments to pom.xml? So that we can merge this PR and it doesn't stay open for long time. And then separate PR to be opened focused only issues.jenkins-ci.org/browse/JENKINS-62800?
If you agree, can you please adjust the PR title/description to match that and we merge it.

@timja

timja commented Sep 24, 2020

Copy link
Copy Markdown
Member Author

@timja thanks for updates here. Will be ok updating the current PR to be focused only on Java 8 migration and adjustments to pom.xml? So that we can merge this PR and it doesn't stay open for long time. And then separate PR to be opened focused only issues.jenkins-ci.org/browse/JENKINS-62800?
If you agree, can you please adjust the PR title/description to match that and we merge it.

done

@vgaidarji vgaidarji self-assigned this Sep 24, 2020
@vgaidarji

vgaidarji commented Sep 24, 2020

Copy link
Copy Markdown
Contributor

@timja I've updated the PR description and was about to do final testing before merge but CI is not passing and seems like there's some issue with update to 4.7 version of Jenkins plugins performed here #63 (comment).
image

I have a custom step for code coverage calculation and upload in Jenkinsfile and it fails to find org.jenkins-ci.plugins:plugin:pom:4.7 dependency.

15:49:01  [ERROR]     Non-resolvable parent POM for org.jenkins-ci.plugins:plot:2.1.9-SNAPSHOT: 
Could not find artifact org.jenkins-ci.plugins:plugin:pom:4.7 in central (https://repo.maven.apache.org/maven2) 
and 'parent.relativePath' points at no local POM @ line 4, column 11 -> [Help 2]

@timja

timja commented Sep 24, 2020

Copy link
Copy Markdown
Member Author

probably would be easier to just change to buildPlugin? it has code coverage support built in, using code-coverage-api plugin rather than codecov

@codecov

codecov Bot commented Jan 23, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #63 (0a45c09) into master (7e38624) will increase coverage by 0.27%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #63      +/-   ##
============================================
+ Coverage     43.38%   43.66%   +0.27%     
  Complexity      190      190              
============================================
  Files            18       18              
  Lines          1263     1255       -8     
  Branches        192      192              
============================================
  Hits            548      548              
+ Misses          651      643       -8     
  Partials         64       64
Impacted Files Coverage Δ Complexity Δ
src/main/java/hudson/plugins/plot/Plot.java 26.01% <0.00%> (+0.14%) 30.00 <0.00> (ø)
src/main/java/hudson/plugins/plot/Series.java 81.81% <ø> (ø) 10.00 <0.00> (ø)
...c/main/java/hudson/plugins/plot/SeriesFactory.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
src/main/java/hudson/plugins/plot/CSVSeries.java 60.89% <0.00%> (+0.38%) 31.00% <0.00%> (ø%)
src/main/java/hudson/plugins/plot/PlotReport.java 46.53% <0.00%> (+0.45%) 10.00% <0.00%> (ø%)
src/main/java/hudson/plugins/plot/XMLSeries.java 72.53% <0.00%> (+0.50%) 36.00% <0.00%> (ø%)
...ain/java/hudson/plugins/plot/PropertiesSeries.java 58.82% <0.00%> (+1.68%) 4.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2844c00...4147de8. Read the comment docs.

@timja
timja requested a review from vgaidarji January 23, 2021 14:35
@timja timja mentioned this pull request Feb 28, 2021
Merged
14 tasks
timja
timja commented Feb 28, 2021
View reviewed changes
Comment thread pom.xml Outdated
Comment thread pom.xml Outdated
@timja
timja merged commit e35fb40 into jenkinsci:master Feb 28, 2021
@timja
timja deleted the modernise-pom branch February 28, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vgaidarji vgaidarji Awaiting requested review from vgaidarji

Assignees

@vgaidarji vgaidarji

Labels

dependencies Dependencies update

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@timja @vgaidarji