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

Apply Develocity Maven extension only if not already applied #393

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

welandaz
Copy link
Member

@welandaz welandaz commented Feb 8, 2024

If a project already has Develocity Maven extension applied, we shouldn't override it with our injection.

If Develocity Maven extension injection is enabled, we will check the .mvn/extensions.xml after checkout for the presence of said extension and disable the injection.

@welandaz welandaz requested review from alextu and c00ler February 8, 2024 08:18
@welandaz welandaz self-assigned this Feb 8, 2024
}

static MavenExtensions fromFilePath(FilePath extensionsFile) {
try(InputStream inputStream = extensionsFile.read()) {
Copy link
Member Author

@welandaz welandaz Feb 8, 2024

Choose a reason for hiding this comment

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

Closing the InputStream here as it's not clear whether DocumentBuilder.parse() will do it

private static boolean mavenExtensionAlreadyApplied(InjectionConfig config, FilePath workspace) throws IOException, InterruptedException {
if (!config.isInjectMavenExtension()) {
return false;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

If injection of Maven extension is tuned off, we don't need to perform this check and simply leave things as they are

Copy link
Contributor

@alextu alextu left a comment

Choose a reason for hiding this comment

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

Good job!
Some notes:

  • I've tested it manually with a docker agent, works fine for both freestyle and pipeline
  • This is kind of breaking change, ie if users had the injection configured on projects, it won't publish anymore to the globally configured DV server. I assume it's ok if it's properly communicated in release notes?


String expr = String.format(EXTENSION_XPATH_EXPR, coordinates.groupId(), coordinates.artifactId());
try {
XPathExpression exprCompiled = XPATH.compile(expr);
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we would pay a small price to compile it every time, but it looks like they already have some internal optimization: first compile call takes about ~6ms, next one 0ms

Copy link
Member Author

@welandaz welandaz Feb 8, 2024

Choose a reason for hiding this comment

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

The method expects any Maven Coordinates, so we can test it easily.

I think it's ok for our use case

@welandaz
Copy link
Member Author

welandaz commented Feb 8, 2024

Good job! Some notes:

  • I've tested it manually with a docker agent, works fine for both freestyle and pipeline
  • This is kind of breaking change, ie if users had the injection configured on projects, it won't publish anymore to the globally configured DV server. I assume it's ok if it's properly communicated in release notes?

Right, that would be the case for Maven ones. The "enforce url" feature only works when using Gradle plugin from what I can see, so there would be no way to override it for Maven extension use case.

We can introduce it though in a separate PR, probably makes sense even for the next release to mitigate this somehow

@alextu
Copy link
Contributor

alextu commented Feb 8, 2024

Good job! Some notes:

  • I've tested it manually with a docker agent, works fine for both freestyle and pipeline
  • This is kind of breaking change, ie if users had the injection configured on projects, it won't publish anymore to the globally configured DV server. I assume it's ok if it's properly communicated in release notes?

Right, that would be the case for Maven ones. The "enforce url" feature only works when using Gradle plugin from what I can see, so there would be no way to override it for Maven extension use case.

We can introduce it though in a separate PR, probably makes sense even for the next release to mitigate this somehow

That's a good point, I think it makes sense to release both, so when the enforce url is true, the extension is taken from the project but the URL is overridden.

@welandaz welandaz merged commit 54b3bad into master Feb 8, 2024
22 checks passed
@welandaz welandaz deleted the welandaz/check-for-extensions-xml branch February 8, 2024 18:33
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.

2 participants