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

Add Dependecies only in specific modules #4453

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcel-gepardec
Copy link
Contributor

A new option to specify a module name in the existing AddDependency recipe has been added. There the dependency will only be added in the specific module.

What's changed?

Added the option "specificModuleName" to the AddDependency Recipe. If "specificModuleName" is added, the dependency will only be added in the specific module and not in the root POM.

Example:

---
type: specs.openrewrite.org/v1beta/recipe
name: com.yourorg.AddDependencyExample
displayName: Add Maven dependency example
recipeList:
    - org.openrewrite.maven.AddDependency:
          groupId: javax.persistence
          artifactId: javax.persistence-api
          version: 2.2
          specificModuleName: test1

Before:

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <parent>
        <groupId>org.example</groupId>
        <artifactId>rootTest</artifactId>
        <version>${revision}</version>
        <relativePath>../pom.xml</relativePath>
    </parent>
    <artifactId>test1</artifactId>
    <packaging>pom</packaging>
</project>

After:

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <parent>
        <groupId>org.example</groupId>
        <artifactId>rootTest</artifactId>
        <version>${revision}</version>
        <relativePath>../pom.xml</relativePath>
    </parent>
    <artifactId>test1</artifactId>
    <packaging>pom</packaging>
    <dependencies>
        <dependency>
            <groupId>javax.persistence</groupId>
            <artifactId>javax.persistence-api</artifactId>
            <version>2.2</version>
        </dependency>
    </dependencies>
</project>

What's your motivation?

A work colleague discovered that the option "onlyIfUsing" is not very consistent and will miss some types. So the solution was to add an option where you can specifically mention the module name in which the dependency should be added.

@timtebeek timtebeek marked this pull request as draft August 28, 2024 11:03
@timtebeek
Copy link
Contributor

Hmm; wouldn't this be better served with a precondition? We've introduced that concept to prevent us from having to add similar options to multiple recipes, for the long term maintainability of the project.

In general we want recipes to be able to run with the same configuration across multiple projects. An option like specificModuleName makes this hyper specific to a single project, or projects following a very strict layout. That overfit is something we'd like to avoid.

@timtebeek timtebeek added the question Further information is requested label Aug 28, 2024
@sgartner03
Copy link

sgartner03 commented Aug 28, 2024

Hi @timtebeek , the condition onlyIfUsing == null && getResolutionResult().getParent() != null && acc.pomsDefinedInCurrentRepository.contains(getResolutionResult().getParent().getPom().getGav()) in AddDependency Line 215 prevents the dependency from being added in some cases.

if(onlyIfUsing == null && getResolutionResult().getParent() != null && acc.pomsDefinedInCurrentRepository.contains(getResolutionResult().getParent().getPom().getGav())) {
return maven;
}

I tried to use it with a precondition at first. However, because I can't specify onlyIfUsing and my parent POM actually is defined in my current repository, the dependency never gets created. We thought, this would be the simplest increment without changing existing behavior. Of course, we're open for other suggestions ^^

For my own use-case, I've created my own recipe that calls the underlying AddDependencyVisitor with a precondition. At least, I could contribute my search recipe for selecting a module. Then @marcel-gepardec only has to change the condition in question.

@timtebeek
Copy link
Contributor

timtebeek commented Aug 29, 2024

I'm not sure fully see the use case here, but that could also be because I have limited time to dive into the specifics. I do still feel that adding specificModuleName might not be the answer though; again: because recipes ought to be run generically, not specific to any one project structure.

What's the practical use here at all of adding a dependency to a specific module without any onlyIfUsing? Could that be better served with a dedicated recipe instead as outlined above?

And don't get me wrong: I appreciate all the contributions, even if I don't always find time to review them within a week. In this case I'm mostly wondering if it even is a case we should generalize, and then maintain those options, or whether a specific in house recipe could be better.

@sgartner03
Copy link

Hi, I already understood your doubts about specificModuleName. However, I am not able to just pair this recipe with a precondition for selecting a single module or multiple modules. That's because of the condition I've mentioned in my previous comment. I see two use-cases for adding a dependency without onlyIfUsing:

  • Dependencies without references in the code (Implementations of the Jakarta specification, for example)
  • Recipes that add some code using a new dependency

For these use-cases, You'd have to call AddDependencyVisitor manually in a java recipe. My proposition is to:

  • Change the condition onlyIfUsing == null && getResolutionResult().getParent() != null && acc.pomsDefinedInCurrentRepository.contains(getResolutionResult().getParent().getPom().getGav()) to allow this case
  • Contribute a search recipe for selecting maven modules.

Again, currently I can't add a dependency, when:

  • onlyIfUsing is not specified
  • AND the parent POM is defined in my repository

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants