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

[JENKINS-58028] Add GMavenPlus version #42

Closed
wants to merge 3 commits into from

Conversation

bitwiseman
Copy link

Eventually will replace gmaven with gmavenplus.

This commit should not change downstream behavior aside from setting the version of GMavenPlus.

Eventually will replace gmaven with gmavenplus.

This commit should not change downstream behavior aside from setting the version of GMavenPlus.
@bitwiseman bitwiseman changed the title [JENKINS-58028] Add GMavenPlus to plugins [JENKINS-58028] Add GMavenPlus version Jun 19, 2019
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.

Please provide a downstream PR with an update which uses GMavenPlus

@bitwiseman
Copy link
Author

@oleg-nenashev
See jenkinsci/plugin-pom#209 and related for examples.
However, there's no downstream changes needed for this change. This only enables use of gmavenplus with a consistent version. I'm not even sure what projects are directly downstream from this pom. Suggestions?

@oleg-nenashev
Copy link
Member

Right, I did not notice that the execution section was for lifecycleMappingMetadata

@oleg-nenashev
Copy link
Member

Any plans to finish it @bitwiseman ?

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.

+1 for getting it over the line. Taking the experience with GMavenPlus in other components, I think it is ready to go

@bitwiseman
Copy link
Author

@oleg-nenashev I've been heads down on Matrix. I'll get back to it when that clears.

@oleg-nenashev
Copy link
Member

Should we go ahead and replace GMaven @jenkinsci/core ? AFAICT it is no longer maintained, and the same patch has worked quite well for plugins

@oleg-nenashev oleg-nenashev requested a review from a team May 30, 2021 16:36
@jglick
Copy link
Member

jglick commented Jun 1, 2021

Merge conflicts here.

This is OK though I would still generally recommend components not use Groovy sources to begin with. pipeline-model-definition is the main offender (among Maven-based repos at least).

@basil
Copy link
Member

basil commented Dec 13, 2021

Do we need this in the parent POM? I looked around for usages of GMaven in any POMs whose parent is this repository and the only two I could find were acceptance-test-harness and lib-groovy-guice-binder (itself only consumed by acceptance-test-harness). If this is only used by acceptance-test-harness then perhaps we should close this PR and just migrate acceptance-test-harness and lib-groovy-guice-binder to GMavenPlus directly?

@bitwiseman
Copy link
Author

@basil
Having this version controlled in the POM would be better.

As Oleg noted GMaven is no longer maintained and this replaces that. If anyone uses GMaven, this makes it easy for them to switch and standardized the version of GMavenPlus that gets used.

@bitwiseman bitwiseman marked this pull request as ready for review December 13, 2021 19:21
pom.xml Outdated Show resolved Hide resolved
@basil
Copy link
Member

basil commented Dec 13, 2021

Having this version controlled in the POM would be better.

I trust that you've looked into this, but I don't quite see the reason why it would be better. From a different perspective, it could be argued that slimming down the dependency tree of the POM would be better. Either way, I have no strong preference, but I do have a preference that we file downstream PRs in acceptance-test-harness and lib-groovy-guice-binder adapting them to the changes in this PR.

Co-authored-by: Liam Newman <[email protected]>
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.

-0 I suppose. Harmless, though most components should not be using Groovy sources, and for those that do, I see no particular advantage in managing the mojo version here.

@bitwiseman
Copy link
Author

-0 I suppose. Harmless, though most components should not be using Groovy sources, and for those that do, I see no particular advantage in managing the mojo version here.

For the projects that do, it is better that we guide them away from GMaven to GMavenPlus and to not managing the version of the plugin on their own.

@jglick
Copy link
Member

jglick commented Dec 13, 2021

guide them away from GMaven to GMavenPlus

Sure.

to not managing the version of the plugin on their own

Why? Usually we add a Maven plugin version to a POM either because it is widely used, or because we define some configuration for it, or because unexpected updates could have bad interactions with something else we define. Miscellaneous plugins used by particular components for idiosyncratic reasons (Shade, for example) do not particularly benefit from being defined in some parent. It just makes for one extra level of indirection in Dependabot.

@jglick
Copy link
Member

jglick commented Dec 13, 2021

As of jenkinsci/acceptance-test-harness#719 we are down to lib-groovy-guice-binder, an odd little library which seems to be a dependency of acceptance-test-harness.

@timja
Copy link
Member

timja commented Dec 14, 2021

@jglick
Copy link
Member

jglick commented Dec 14, 2021

Yes various ATH scripts define CONFIG as per this implementation; example.

@basil
Copy link
Member

basil commented Nov 2, 2022

There has been no interest in this during the intervening year.

@basil basil closed this Nov 2, 2022
@jglick
Copy link
Member

jglick commented Nov 2, 2022

In fact we have been moving to remove src/{main,test}/groovy/ in various repos.

@basil basil mentioned this pull request Nov 8, 2022
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.

6 participants