Skip to content
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

driver: do not insert "platform" as driver-opt #475

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

tonistiigi
Copy link
Member

@tonistiigi tonistiigi commented Dec 15, 2020

Addresses docker/setup-buildx-action#45

Simple repro:

$ buildx create --platform linux/amd64 --use
$ buildx build - <<EOF
from scratch
EOF

Since #370 a platform driver-opt was automatically inserted with the value specified by --platform flag on regardless of the type of driver, even though it was only used in the kubernetes driver. However, because the docker-container driver is pedantic about the options being passed, it errored out.

Another side-effect I suspect is that with the kubernetes driver it was now possible to specify the platforms in two different ways: --driver-opt platform=... and --platform.

This patch reverts completely the platform driver-opt and instead ensures the platforms information is passed onto the kubernetes driver via variables.

Signed-off-by: Tibor Vass [email protected]

cc @morlay

@tiborvass tiborvass force-pushed the fix_create_platform_regression branch from 5aeca47 to c994b94 Compare December 15, 2020 06:57
Addresses docker/setup-buildx-action#45

Simple repro:
```
$ buildx create --platform linux/amd64 --use
$ buildx build - <<EOF
from scratch
EOF
```

Since docker#370 a `platform` driver-opt was automatically inserted with the value specified by `--platform` flag on regardless of the type of driver, even though it was only used in the kubernetes driver. However, because the docker-container driver is pedantic about the options being passed, it errored out.

Another side-effect I suspect is that with the kubernetes driver it was now possible to specify the platforms in two different ways: `--driver-opt platform=...` and `--platform`.

This patch reverts completely the `platform` driver-opt and instead ensures the platforms information is passed onto the kubernetes driver via variables.

Signed-off-by: Tibor Vass <[email protected]>
@tiborvass tiborvass force-pushed the fix_create_platform_regression branch from c994b94 to 381dc8f Compare December 15, 2020 07:10
@tonistiigi
Copy link
Member Author

LGTM

@morlay
Copy link
Collaborator

morlay commented Dec 15, 2020

This pr works well. with kubernetes driver

docker buildx create --use --name buildkit --platform=linux/amd64 --node=buildkit-amd64 --driver=kubernetes --driver-opt=namespace=gitlab
docker buildx create --append --name buildkit --platform=linux/arm64 --node=buildkit-arm64 --driver=kubernetes --driver-opt=namespace=gitlab

Copy link
Collaborator

@tiborvass tiborvass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotta try approving my own PR :P

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tonistiigi tonistiigi merged commit 11057da into docker:master Dec 15, 2020
@tiborvass tiborvass deleted the fix_create_platform_regression branch December 15, 2020 07:38
codefromthecrypt pushed a commit to openzipkin/docker-alpine that referenced this pull request Dec 17, 2020
In #11, I tried to find a configuration that works both in Docker 19
and 20. However, this breaks the build.

This reverts the change as there are bugs popping up related on Docker
20 anyway, such as docker/buildx#475

We use Docker from the OS distribution, so unlikely to accidentally
walk into a Docker 20. In other words, when it becomes stable, we can
try again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants