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 recipe for adding/replacing a Maven runtime config #4363

Merged
merged 6 commits into from
Jul 29, 2024

Conversation

bmuschko
Copy link
Contributor

What's changed?

Introduces a new recipe for adding and replacing Maven runtime configuration in .mvn/maven.config or .mvn/jvm.config.

What's your motivation?

It's important for Maven projects to standardize on certain runtime flags and make them available through checked-in files the SCM. All developers that check out the code from SCM will automatically apply those flags. For example, you may want to execute the Maven build in parallel for better build performance and therefore introduce -T=1C in maven.config.

Anything in particular you'd like reviewers to focus on?

  • The Maven documentation is not very specific which notation should be used for separating a flag from the assigned value if there is one (no separation, whitespace, = character. In fact, you will find all notations across different pieces of documentation. The implementation allows for providing this option.
  • The Maven documentation indicates that new lines should be used to separate configuration for maven.config. For jvm.config, you should use a whitespace. The implementation takes this into consideration.
  • Currently, the implementation does not take into consideration the alternative flag notation e.g. -U vs. --update-snapshots. Please advise if you'd need this case to be handled with the initial PR and optimally "how".

@timtebeek timtebeek self-requested a review July 28, 2024 06:54
@timtebeek timtebeek added the recipe Requested Recipe label Jul 28, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-gradle/src/main/groovy/RewriteSettings.groovy
    • lines 31-36
  • rewrite-java-test/src/test/java/org/openrewrite/java/UseStaticImportTest.java
    • lines 336-336

bmuschko and others added 2 commits July 28, 2024 10:35
…nfig.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…nfigTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-gradle/src/main/groovy/RewriteSettings.groovy
    • lines 31-36
  • rewrite-java-test/src/test/java/org/openrewrite/java/UseStaticImportTest.java
    • lines 336-336

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-gradle/src/main/groovy/RewriteSettings.groovy
    • lines 31-36

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-gradle/src/main/groovy/RewriteSettings.groovy
    • lines 31-36

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great start @bmuschko ; I've added a small polishing commit, mostly to make things smaller and more idiomatic. Feel free to go over the changes and let me know if you have any question.

No need to handle configuration aliases just yet; but we can see how that turns out in practice.

…nfig.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-gradle/src/main/groovy/RewriteSettings.groovy
    • lines 31-36

@timtebeek timtebeek merged commit e15e821 into openrewrite:main Jul 29, 2024
2 checks passed
@bmuschko
Copy link
Contributor Author

@timtebeek Thanks for the polishing and merging. All makes sense and gives additional guidance for future PRs.

@bmuschko bmuschko deleted the bm/maven-runtime-config branch July 29, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Requested Recipe
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants