TestClusters: Generic artifact injection for unreleased versions #41208
Conversation
This PR removes coupling between BwcVersions and testclusters. The goal is to have TestClusters deal with released artifacts only, with support for a map that tells it where to look for others for the special case of our build that tests unreleased versions. The main build script now configures this map on all testclusters from the build.
alpar-t
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-core-infra |
| } | ||
|
|
||
|
|
||
| subprojects { |
There was a problem hiding this comment.
I'm not sure this "decoupling" is really all that much better. I certainly think the registering of artifacts bit is better but realitically, these things are coupled, we've just moved the logic into a more obscure place.
I think the awkwardness is around how we fetch an instance of versions. I think we could really simplify this by moving this into a static method on BwcVersions. Something like getInstance(). Then we could get at this from anywhere we want without having to awkwardly get at a extra property on the root project.
There was a problem hiding this comment.
Making it easier to reference will just increase the coupling.
I don't see why you would think they are coupled, with the registering of the artifacts testclusters don't care at all where these are coming from and weather these were released or not, and the bits that do care moved closer together in the build script.
Also this will offer more clarity when ruining in external plugins that can only deal with released versions. I also think it makes it easier to understand and reason about what testclsuters does as it doesn't mix in related functions like dealing with unreleased artifacts.
There was a problem hiding this comment.
My concern is that's its more difficult to reason about where test clusters is resolving these artifacts from because they are registered outside the plugin. Injection in Gradle plugins is simply awkward and there's no good solution for it. So the issue with exposing some API is that it could be called from anywhere. This is probably fine but I generally like to avoid adding more stuff to the root build.gradle which is already getting large and doesn't benefit from static compilation and type safety like stuff in buildSrc does.
| @@ -194,7 +194,14 @@ if (project != rootProject) { | |||
| } | |||
|
|
|||
| String localDownloads = "${rootProject.buildDir}/local-downloads" | |||
| // TODO: keep a single copy once we have testclusters for example plugins | |||
There was a problem hiding this comment.
I'm not sure I follow. Why do we need two copies of these?
There was a problem hiding this comment.
Some of the tests like the example plugins still use ClusterFormationTasks , but with this change, the Version class doesn't encode -SNAPSHOT. The result is that one will look for 8.0.0 and the other for 8.0.0-SNAPSHOT in the artifact name. This won't matter once we have everything on testclusters and we can clean it up then.
There was a problem hiding this comment.
Can you please remove the whitespace changes from the root build.gradle? I know you mentioned this was unintentional in a comment, but I cannot figure out which changes are actually to that file.
I agree with Mark that decoupling like this seems overkill. In fact, I want to add more coupling in some ways. We need to pass extra information to these test clusters to improve the reproduce line to include the hash of what was built for that bwc version. This decoupling seems like additional abstraction that will only make that work more difficult, but not actually make the code any less coupled.
|
ok, I see your point on these being actually coupled, but then let's look at making this more explicit. My distaste is for how this is achieved using ext properties. I also don't like it that this part is not really testable since it requires the entire project to figure out and build bwc versions ( but this part is not solved in this PR either ). |
|
I really wish Gradle's service registry stuff, or some version of it, would become public API. The ability to register global services with explicit scopes that could be injected into managed code would be great. There's great need for this and we use hacks like attaching extra properties to the root project as a way to get around this. |
|
Closing this in favor of #41504 |
This PR removes coupling between BwcVersions and testclusters.
The goal is to have TestClusters deal with released artifacts only,
with support for a map that tells it where to look for others for
the special case of our build that tests unreleased versions.
The main build script now configures this map on all testclusters from
the build.
I accidentally reformatted the main
build.gradlethe actual change is only at the end of the file.