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

Disable injection when custom extensions are detected #398

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

alextu
Copy link
Contributor

@alextu alextu commented Feb 22, 2024

Follow up of #393

This PR adds 2 new fields to the Develocity Maven injection:

image

Those are used in case built projects have defined custom Develocity derived extensions:

  • If the user specifies a GAV in "Develocity Maven Extension Custom Coordinates" field, the Develocity injection will be disabled if a corresponding extension is found in .mvn/extensions of the currently built project.
  • Similarly, if the user specifies a GAV in "Common Custom User Data Maven Extension Custom Coordinates" field, the CCUD injection will be disabled if a corresponding extension is found in .mvn/extensions of the currently built project.

It's a bit complex in terms of implementation since we need to parse the existing MAVEN_OPTS looking for the maven ext classpath system property, and filter it accordingly.
We might want to revisit this since it looks like the ScmListener we use is called in many cases, even when no Scm is configured...

Testing done

Unit tests and a few integration tests. Some cases cannot easily be setup as integration tests since they would require "real" custom Develocity extensions.
I also tested manually the 2 cases:

  • custom extension defined in the built project and corresponding "Develocity Maven Extension Custom Coordinates" field set => DV extension is not applied. The CCUD extension is still applied if "Enable Common Custom User Data Maven extension auto-injection" is checked.
  • custom extension defined in the built project and corresponding "Common Custom User Data Maven Extension Custom Coordinates" field set => CCUD extension is not applied. Note that the DV extension is still applied, although most likely, it should also be defined locally; in that case, it's not applied.

Submitter checklist

Preview Give feedback

@alextu alextu requested a review from welandaz February 22, 2024 15:48
@alextu alextu changed the title Disable injection when custom extension are detected Disable injection when custom extensions are detected Feb 22, 2024
Copy link
Member

@welandaz welandaz left a comment

Choose a reason for hiding this comment

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

It is a bit convoluted now, but in order to simplify it, we need to rewrite the way injection is done in general.
Good work otherwise 👍

public FormValidation doCheckCcudExtensionCustomCoordinates(@QueryParameter String value) {
String coord = Util.fixEmptyAndTrim(value);
return coord == null || MavenCoordinates.isValid(coord) ? FormValidation.ok() : FormValidation.error(Messages.InjectionConfig_InvalidMavenExtensionCustomCoordinates());
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe we can extract the logic of both of these methods into a private method and reuse it


static boolean isUnix(Computer computer) {
return computer == null || Boolean.TRUE.equals(computer.isUnix());
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd also make this class final and add private constructor

public final class SystemProperty {

private final Key key;
private final String value;
private static final Pattern SysPropPattern = Pattern.compile("-D(.*)=(.*)");
Copy link
Member

Choose a reason for hiding this comment

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

I'd pull it to the top of the class and adjsut the name to SYS_PROP_PATTERN to stick to the convention

@alextu alextu merged commit 6439fd2 into master Feb 27, 2024
22 checks passed
@alextu alextu deleted the atual/custom-coordinates branch February 27, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants