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.

Sign up for GitHub

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

Jump to bottom

Add validate-hpi which checks core version for dependencies before packaging #191

Merged
merged 3 commits into from
Oct 9, 2020
Merged

Add validate-hpi which checks core version for dependencies before packaging #191

merged 3 commits into from
Oct 9, 2020

Conversation

Vlatombe
Copy link
Member

Adds check that the required core version is higher or equal to the core version required by plugin dependencies.

This is indirectly done by the injected tests already but it requires to run the tests and it is much longer.

@oleg-nenashev oleg-nenashev self-requested a review August 12, 2020 12:49
@MarkEWaite MarkEWaite mentioned this pull request Aug 12, 2020
Closed
@oleg-nenashev oleg-nenashev mentioned this pull request Oct 7, 2020
Merged
timja
timja reviewed Oct 7, 2020
View reviewed changes
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

would be nice to also see a test that show this doesn't break exact version matches

e.g. 2.222.4 is fine in this case.

src/it/check-core-version/pom.xml Outdated Show resolved Hide resolved
@Vlatombe
PR Review from @timja
65c4bef 
* Add a test with success case (exact version match)
* Define jenkins.version in the pom for clarity and safeguard against
parent pom bump
@Vlatombe Vlatombe requested a review from timja October 8, 2020 07:50
timja
timja approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Thanks

@timja timja requested a review from jglick October 8, 2020 08:09
oleg-nenashev
oleg-nenashev requested changes Oct 9, 2020
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Currently ValidateMojo might be used for non-HPI packaging. With this change HPI packaging will be required IIUC the implementation. Please correct me if I am wrong. If not, I suggest creating a new validate-hpi mojo for the new checks.

src/main/java/org/jenkinsci/maven/plugins/hpi/ValidateMojo.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/maven/plugins/hpi/ValidateMojo.java Outdated
MavenArtifact maxCoreVersionArtifact = null;
VersionNumber maxCoreVersion = new VersionNumber("0");

for (MavenArtifact artifact : Sets.union(getProjectArtfacts(), getDirectDependencyArtfacts())) {
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker: Note that it does not take transitive dependencies into account. All plugins in the dependency tree will have to adopt the new Maven HPI Plugin version to prevent from errors in transitive deps. Related to #195 which also needs a full plugin dependency tree resolution.

src/main/java/org/jenkinsci/maven/plugins/hpi/ValidateMojo.java Outdated Show resolved Hide resolved
@jglick
Copy link
Member

jglick commented Oct 9, 2020

Note that you might expect this to be done by RequireUpperBoundsDeps but you would be wrong, since jenkins-core is a provided dep and thus not transitive.

oleg-nenashev
oleg-nenashev approved these changes Oct 9, 2020
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

LGTM. There could be a more fancy documentation, but it should be good enough for adoption in parent POMs.

@oleg-nenashev oleg-nenashev changed the title Check core version for dependencies before packaging Add validate-hpi which checks core version for dependencies before packaging Oct 9, 2020
@oleg-nenashev oleg-nenashev mentioned this pull request Oct 9, 2020
Open
@oleg-nenashev oleg-nenashev merged commit 991ffbb into jenkinsci:master Oct 9, 2020
@Vlatombe Vlatombe deleted the check-core-version branch October 9, 2020 14:48
@oleg-nenashev
Copy link
Member

https://github.com/jenkinsci/maven-hpi-plugin/releases/tag/maven-hpi-plugin-3.16 is out @Vlatombe . I'd guess we need to enable the new mojo by default in Jenkins Plugin POM.

@Vlatombe
Copy link
Member Author

Vlatombe commented Oct 9, 2020

@oleg-nenashev Should be automatic since I added it to the hpi packaging https://github.com/jenkinsci/maven-hpi-plugin/pull/191/files#diff-c9fbb41a606335288946706b0925b227R14

@oleg-nenashev
Copy link
Member

right, let's see. An integration test would be useful in Plugin POM, just in case

@jglick
Copy link
Member

jglick commented Oct 9, 2020

An integration test would be useful in Plugin POM

Perhaps, though as noted in #191 (comment), a misconfigured plugin build will already fail—it will just take a bit longer.

FWIW I keep a custom shortcut in NetBeans to help test pom.xml edits:

mvn clean test -Dtest=InjectedTest

@zbynek zbynek mentioned this pull request Oct 15, 2020
Merged
@Vlatombe Vlatombe mentioned this pull request Oct 15, 2020
Merged
@Vlatombe Vlatombe mentioned this pull request Oct 5, 2021
Merged
6 tasks
@jglick jglick mentioned this pull request Jul 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timja timja timja approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@jglick jglick Awaiting requested review from jglick

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Vlatombe @jglick @oleg-nenashev @timja