-
Notifications
You must be signed in to change notification settings - Fork 292
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
AddManagedDependency
should allow version placeholder properties
#4032
Comments
Thanks for the suggestion @dpozinen ; how would you envision this to work both in terms of configuration, and in terms of when the recipe runs? Because I have quite a few questions when I think through how to implement this, and I'll list a few below. Right now the recipe already has quite an array of options: https://docs.openrewrite.org/recipes/maven/addmanageddependency
I'm thinking if we add option to indicate that the version should be placed in a property, then the naming of that property is going to be very much specific to one's particular taste. Some projects prefer We're constantly trying to balance feature requests with usability and maintainability by thinking through some of these above edge cases. Since versions can be updated through recipes, we tend to favor just having them where they are used rather than add indirection. If you do want to keep managed dependency versions in properties consistently then perhaps a dedicated recipe could make sense. That also has the benefit of not further complicating the logic for AddManagedDependency, so it's something to consider. |
Hey @dpozinen, #3992 should have fixed that, but the following still does not work out of the box: type: specs.openrewrite.org/v1beta/recipe
name: org.ammachado.MyCustomRecipe
displayName: My custom recipe
description: Fix all the things.
recipeList:
- org.openrewrite.maven.ChangePropertyValue:
key: quarkus.platform.group-id
newValue: io.quarkus.platform
addIfMissing: "True"
- org.openrewrite.maven.ChangePropertyValue:
key: quarkus.platform.artifact-id
newValue: quarkus-bom
addIfMissing: "True"
- org.openrewrite.maven.ChangePropertyValue:
key: quarkus.platform.version
newValue: 3.2.3.Final
addIfMissing: "True"
- org.openrewrite.maven.AddManagedDependency:
groupId: ${quarkus.platform.group-id}
artifactId: ${quarkus.platform.artifact-id}
version: ${quarkus.platform.version}
scope: import
type: pom
addToRootPom: "True" The properties will be added, but the dependency will fail to resolve, because the placeholders were not resolved. I think that Updated exampletype: specs.openrewrite.org/v1beta/recipe
name: org.ammachado.MyCustomRecipe
displayName: My custom recipe
description: Fix all the things.
recipeList:
- org.openrewrite.maven.ChangePropertyValue:
key: quarkus.platform.group-id
newValue: io.quarkus.platform
addIfMissing: "True"
- org.openrewrite.maven.ChangePropertyValue:
key: quarkus.platform.artifact-id
newValue: quarkus-bom
addIfMissing: "True"
- org.openrewrite.maven.ChangePropertyValue:
key: quarkus.platform.version
newValue: 3.2.3.Final
addIfMissing: "True"
- org.ammachado.ForceMavenModelUpdate
- org.openrewrite.maven.AddManagedDependency:
groupId: ${quarkus.platform.group-id}
artifactId: ${quarkus.platform.artifact-id}
version: ${quarkus.platform.version}
scope: import
type: pom
addToRootPom: "True"
|
oh nice, #3992 indeed added this possibility, things move fast lol; apologies for not checking first. And indeed what you're describing @ammachado is what we're trying to do (add property and then use it), will try your workaround for maven model update, thanks! |
Thanks for chiming in @ammachado ! Indeed that As for the rewrite/rewrite-core/src/main/java/org/openrewrite/config/YamlResourceLoader.java Lines 322 to 326 in fe15bdb
|
Hello @ammachado @timtebeek! I think that the fix doesn't fully work as it should. Recipe example:
result:
In this case the placeholder is not saved, but replaced with the some version. The reason is that the maybeUpdateModel() method is called after an attempt has been made to make changes to the pom file. If I add second AddProperty and AddManagedDependency in this recipe, then it'll work in the second managed dependency as it should, because the maybeUpdateModel() method, which was called in the first AddManagedDependency, helped in this. In this case, it seems to me that it's necessary to modify AddProperty and ChangePropertyValue by adding maybeUpdateModel() there after properties added/changed. |
That sounds reasonable, yes thanks; would you want to push that up as a pull request? |
@mankoffs your example need the extra recipe from #4032 (comment). I created another PR, #4069, to update the model automatically without the need of extra recipes. @timtebeek can you please review it? |
What problem are you trying to solve?
I'm trying to add a managed dependency via
AddManagedDependency
9/10 times a managed dependency is declared via a version placeholder in a bom to allow overrides. Currently the recipe only accepts semver versions.
Describe the solution you'd like
Accept
${something.version}
and probably validate that such a property exists.The text was updated successfully, but these errors were encountered: