Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Only define Docker pkg tests if Docker is available #47640
Only define Docker pkg tests if Docker is available #47640
Changes from 42 commits
521d41d
c01dd7b
d2dccce
5eb2a28
ec8b085
1f48f44
560adb6
7212a9c
de9bb33
2020084
e8a920a
2d8ca75
1684857
1641b38
54d322c
2d96534
64051ed
6a64065
8cb1389
b0e3a9e
a20b3d4
daec2ad
be25db4
12bb939
31fec0b
5c785c7
2293017
8c819fb
55d4cfc
b8d44c7
dc19fed
e98e4bc
0831884
0a29d8d
048832a
99ded1c
12bd9fb
97fca15
617f494
ffcf939
0b49a7a
611b914
aac11a4
e0010a6
f189ca1
63a758d
3e8d10d
cedacee
ef924fc
9d50aaa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This property is a left over and it's my bad for not removing it yet. See #45034.
Please remove this property here together with the conditionals and the
ext.set('buildDocker'
from bellow it as it's not used, and make sure we don't mention it in any new code we add.The way this is meant to work is that docker is required if you need it to build something, trying to build that something without docker should fail. It's optional for anything involving testing.
We usually get around this by only calling
build
( which includesassemble
) on CI workers that have docker, and callingcheck
when they might not , which doesn't include a full assemble and only builds what it needs for the tests, if no tests can run nothing gets assembled. The logic is implemented in the test fixtures plugin.The docker rest tests are a special case that the test fixtures plugin doesn't fully handle, see
distribution/docker/build.gradle
that has this :We got into this situation because we have platforms for which we can't have docker in CI, but still want to run the rest of the tests. Ideally we would just require docker for development as we rely on it in some many ways these days. (Of course we would have to support test fixtures on mac and windows which we don't yet, but that's a different story).
The situation is a bit different here because we are able to pick when to run and when not to because we know what platform we are running against, but that's only when we are running against vagrant and the subprojects are actually in use. In this case the VMs will have docker as we install it in the Vagrant file, but the host also has to have it to build the image, which our bare metal workers do have, so we are ok to fail, since there's no way to test docker without having it installed.
On the GCP images, we may not have it, so we can't build nor test, anything docker related, so I think we need to keep the same semantics here, avoid pulling in the dependency to build the images and skip the docker related tests ( which we already do ), since this has to do with GCP it's similar to the rest of CI, as in we don't have a way of configuring the platforms that are ok not to have docker, so the only way to implement my earlier suggestion to have a list of platforms where it's ok not to have docker is in CI and it would be similar for every CI job and worker we have, not specific to packaging tests.
I think the easiest way to accomplish this is to skip configuring the distribution altogether in this case, similar to how we deal with docker rest tests.
We don't really care about not having docker on the host running the vagrant tests,
so I don't think we should do anything about it.
As sych the part of the PR that configures the
shouldTestDocker
ext property should be discarded.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.
The typical way to go about this is to register an Gradle extension on the project where the project could provide additional configuration, like weather or not to test on docker.
Aslo, I think we should specifically opt-put instead of opt in as most platforms support it and future platforms are expected to support it.
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.
This is no longer applicable, see my earlier 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.
Not sure what earlier comment you are referring to @atorok.
Also, I think this is going to have to go in an
afterevaluate { }
block to ensure that property has been evaluated.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.
I was referring to this one: #47640 (comment) didn't realize GH will put it after not before.
My point is that we don't need to bother with docker support for the subprojects for when docker is not available in the Vagrant VM ( which is when these are actually used ) . The tests are skipped when there's no docker and there is no need to build an image inside the VM, so we're all good. Not having docker on the host that runs vagrant is ok to error out.