-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Define lifecycle tasks for running different types of packaging tests #54134
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
Define lifecycle tasks for running different types of packaging tests #54134
Conversation
|
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
rjernst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see this will be an easy improvement. I have just a couple comments.
| for (ElasticsearchDistribution distribution : distributions) { | ||
| TaskProvider<?> destructiveTask = configureDistroTest(project, distribution, dockerSupport); | ||
| destructiveDistroTest.configure(t -> t.dependsOn(destructiveTask)); | ||
| Optional.ofNullable(lifecyleTasks.get(distribution.getType())).ifPresent(p -> p.configure(t -> t.dependsOn(destructiveTask))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would this not be present? Shouldn't that be an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right. I had initially anticipated something like the integ test zip type, but we aren't testing that here. And if we do add a type we would probably want this to explode rather than silently ignore. I've removed the optional wrapper here.
|
|
||
| // Create lifecycle tasks so we can run all tests of a given type | ||
| Map<ElasticsearchDistribution.Type, TaskProvider<?>> lifecyleTasks = new HashMap<>(); | ||
| lifecyleTasks.put(Type.DOCKER, project.getTasks().register("destructiveDistroTest#docker")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've used . as the separator in the rest of the tasks in this plugin. Can we reuse that here? It is easier when typing out the task name to not need to add quotes as # requires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced # with ..
rjernst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
I just realized this only adds destructive tasks for use by CI. Can you also add local versions which trigger the appropriate VM tasks? |
Done. |
|
@elasticmachine update branch |
We want to try and cut down the pull request feedback times even more. Right now the long pole is our packaging tests, which take nearly an hour, vs most other checks which are 30 minutes or less. Right now our packaging test builds are structured in a monolithic way, in which we serially test each distribution type for a given target operating system. We could significantly reduce feedback times by splitting these jobs up, and running a job per distribution type.
This PR modifies our distribution testing plugin to add per-type lifecycle tasks which will allow us to structure our CI jobs in the same way. We now create a
docker,archivesandpackageslifecycle task such that we can run all tests for a give distribution type with a single task, or run the whole enchilada withdistructiveDistroTestas we have always done.