Implement automatic CI Visibility configuration for Maven projects#5043
Conversation
nikita-tkachenko-datadog
changed the title
nikita-tkachenko-datadog
added
the
comp: ci visibility
…ty and --add-exports support
15d4369 to
a38f28c
Compare
BenchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases. See unchanged results
|
nikita-tkachenko-datadog
changed the title
albertvaka
left a comment
albertvaka
left a comment
There was a problem hiding this comment.
Is there some automated testing we can do here?
| : MavenPluginVersion.UNKNOWN; | ||
|
|
||
| if (mavenPluginVersion.isLaterThanOrEqualTo(ANNOTATION_PROCESSOR_PATHS_SUPPORTED_VERSION)) { | ||
| String lombokVersion = getLombokVersion(projectDependencies); |
There was a problem hiding this comment.
Is the need for lombok on older Maven versions documented anywhere?
There was a problem hiding this comment.
It's not like we're adding Lombok here, this is about ensuring Lombok does not break if the project is already using it.
In newer Maven compiler plugin versions - the ones that support annotation processor paths - whenever we're adding DD Javac Plugin as annotation processor, Lombok (if it happens to be used in the project) stops working, unless we add it as an annotation processor as well.
This is an edge case that is documented in DD Javac Plugin documentation
| .append(propertyName) | ||
| .append('=') | ||
| .append(e.getValue()) | ||
| .append(System.lineSeparator()); |
There was a problem hiding this comment.
Can we split the arguments by newlines? Is that supported in all OSes? Shouldn't they be just split by spaces?
There was a problem hiding this comment.
Now I have doubts :)
I always used to use newlines when configuring Maven manually, and the surefire plugin code contains logic that replaces all the whitespace character with the literal space.
But now I'm not sure about the older plugin versions, and the documentation says nothing about it.
Replaced it with space to be on the safe side.
There was a problem hiding this comment.
There are a few more places where System.lineSeparator() is used. If it works we can leave it, but let's be consistent and use the same thing everywhere.
There was a problem hiding this comment.
Good catch, I missed those. Changed everything to using spaces now
| } | ||
|
|
||
| File agentJar = AgentUtils.getAgentJar(); | ||
| modifiedArgLine.append("-javaagent:").append(agentJar.toPath()); |
There was a problem hiding this comment.
What happens if you have already configured this manually & we try to auto-configure it on top of that? You might end with two -javaagent arguments. Did you test that scenario? Same for the compiler plugin.
There was a problem hiding this comment.
Yes, I have verified it, since I assumed there can be customers who might just update to the new tracer versions without reading the docs.
The tracer contains a safeguard that protects it from initialising twice, and the compiler plugin does not break as well.
Unit testing seems kind of pointless to me, as verifying that a number of methods was executed on a Maven domain object does not give much confidence in the end result. |
albertvaka
left a comment
albertvaka
left a comment
There was a problem hiding this comment.
Left one last comment about System.lineSeparator(), other than that LGTM 🚀
2 participants
What Does This Do
Updates Maven instrumentation so that CI Visibility configuration is done automatically:
Motivation
Simplifying onboarding for CI Visibility customers