Add Watcher to available rest resources #53620
Conversation
jakelandis
added
:Delivery/Build
|
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
| task.includeCore.set(extension.restTests.getIncludeCore()); | ||
| task.includeXpack.set(extension.restTests.getIncludeXpack()); | ||
| task.coreConfig = project.getConfigurations().create("restTest"); | ||
| task.coreConfig = project.getConfigurations().create("restTestConfig"); |
There was a problem hiding this comment.
We should probalby not be creating these configurations inside this configuration block. The reason being, because we are using the lazy task creation api, that means that sometimes we create these, and sometimes we don't, depending on the requested task graph. When it comes to configuration API elements, we should always create these or we could get funny errors depending on what tasks you ask to run.
| .project(Map.of("path", ":x-pack:plugin", "configuration", "restXpackTests")); | ||
| project.getDependencies().add(task.xpackConfig.getName(), restXPackTestdependency); | ||
| task.dependsOn(task.xpackConfig); | ||
| // watcher |
There was a problem hiding this comment.
I'm not particular excited about us hard-coding these mappings. Are we going to have to add something here for every project that shares tests. Also, if we want to get to a point where we can aggregate all xpack tests (as the clients team has asked about) we'll need a more generic solution here likely based on some convention.
There was a problem hiding this comment.
Are we going to have to add something here for every project that shares tests
Yeah, as-is unfortunately. I agree this is less then ideal, and will get ugly after a few more of these. If you have any suggestions, I can explore (but preferably in a different PR).
| task.coreConfig = project.getConfigurations().create("restTest"); | ||
| task.coreConfig = project.getConfigurations().create("restTestConfig"); | ||
| project.getConfigurations().maybeCreate("restTests"); | ||
| project.getConfigurations().maybeCreate("restXpackTests"); |
There was a problem hiding this comment.
I think we should just use create instead of maybeCreate and remove any instances in projects that are manually creating these. The risk with the lenient approach is that some project declares this configuration for one purpose, and we see it and use it for a different one. This should be explicit. If you create a conflicting configuration we should error.
|
@elasticmachine update branch |
|
thanks Mark! |
Prior to this commit Watcher explicitly copied test between two projects with a copy task. This commit removes the explicit copy in favor of adding the Watcher tests to the available restResources that may be copied between projects. This is how inter-project dependencies should be modeled. However, only Watcher is included here since it is (currently) the only project with inter-project test dependencies. Note - this re-introduces: commit: 4f48e05 with some additional fixes.
Prior to this commit Watcher explicitly copied test between two projects with a copy task. This commit removes the explicit copy in favor of adding the Watcher tests to the available restResources that may be copied between projects. This is how inter-project dependencies should be modeled. However, only Watcher is included here since it is (currently) the only project with inter-project test dependencies.
This commit moves Watcher out of the general purpose RestResourcesPlugin and declares the cross project test resource dependency explicitly. This (explicit dependencies) should be the general model for one-off cases such as this. The name of the x-pack configuration has also been updated to better match the non-x-pack variant. related #53620
This commit moves Watcher out of the general purpose RestResourcesPlugin and declares the cross project test resource dependency explicitly. This (explicit dependencies) should be the general model for one-off cases such as this. The name of the x-pack configuration has also been updated to better match the non-x-pack variant. related elastic#53620
This commit moves Watcher out of the general purpose RestResourcesPlugin and declares the cross project test resource dependency explicitly. This (explicit dependencies) should be the general model for one-off cases such as this. The name of the x-pack configuration has also been updated to better match the non-x-pack variant. related elastic#53620
4 participants
Prior to this commit Watcher explicitly copied test between two
projects with a copy task. This commit removes the explicit copy in favor
of adding the Watcher tests to the available restResources that may be
copied between projects.
This is how inter-project dependencies should be modeled. However, only
Watcher is included here since it is (currently) the only project with
inter-project test dependencies.
Note - this was originally reviewed and merged (only to master) via #53319, where it failed the intake test and was reverted.
This PR reverts the revert and adds 6fa7176. I believe the root cause of the failure was overloading the configuration names, and 6fa7176 should address that issue.
The configurations are as follows:
restTests,restXpackTests,restSpecs, andrestXpackSpecsare all configurations that can be defined per module (e.g. watcher, x-pack plugin). Watcher is defining its ownrestXpackTests(hence the maybeCreate) to express it's tests. Other configurations (from those 4) could also be defined per module to express dependencies. Users are expected to interact with these if they need expose anyone of those 4 things.restTestConfigandrestSpecare configurations used by the plugin and should not be created elsewhere (hence only create). Users are not expected to interact with these.The dependencies represented through
restTests,restXpackTests,restSpecs, andrestXpackSpecsconfigurations per project are added torestTestConfigandrestSpecconfiguration for the plugin. The task only cares about these two configurations, which should be a representation of the super set of all of the test or spec dependencies.