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 options to the Add Gradle Enterprise Maven recipe #3233

Merged
merged 2 commits into from
May 16, 2023

Conversation

alextu
Copy link
Contributor

@alextu alextu commented May 11, 2023

Follow up on #3193 (comment)
This PR adds some optional settings:

  • captureGoalInputFiles
  • uploadInBackground
  • publishCriteria

An optional setting not set will not result in a xml element in the gradle-enterprise.xml configuration, this is implemented by using Jackson xml instead of plain string formatting.

objectMapper.setSerializationInclusion(JsonInclude.Include.NON_ABSENT);
return objectMapper.writeValueAsString(new GradleEnterpriseConfiguration(serverConfiguration, buildScanConfiguration));
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how those kind of exceptions are usually handled in recipes ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing a mix of ctx.getOnError().accept(e), Markup.error(e) and throw new RuntimeException, and possibly others.

I think it will mostly depend on if your recipe will be executed as part of a larger chain, in which case you might want to go for "elegant" handing over just throwing a runtime exception. I'm guessing in this case you're fine with a RuntimeException, since we're also not expecting a lot of different failure modes here.

@rpau rpau requested a review from kunli2 May 11, 2023 13:01
@timtebeek timtebeek added the enhancement New feature or request label May 14, 2023
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.

Looks good to me; do you have anything to add @kunli2 ?

@alextu
Copy link
Contributor Author

alextu commented May 16, 2023

Thanks for reviewing !

@timtebeek
Copy link
Contributor

And thank you for this improvement; I'm going to merge this one as it is; if there's any feedback from Kun we can pick that up separately, as we did after the last one.

@timtebeek timtebeek merged commit 0c6589a into openrewrite:main May 16, 2023
@alextu alextu deleted the atual/mvn-ge-options branch May 16, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants