feat(docker): multi-platform docker builds #54
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
miki725
force-pushed
the
ms/multiplatform
branch
2 times, most recently
from
38ca65b
to
31758fb
Compare
miki725
force-pushed
the
ms/multiplatform
branch
7 times, most recently
from
c7ec0af
to
25f9c5b
Compare
This is a pretty crude approach which: * detects if docker build --push --platform=X,Y is used * if so breaks it into: * docker build --load for each platform * docker push for each platform * docker manifest create * docker manifest push As docker manifest is an experimental feature it is guarded so that if anything fails, docker fallback is used And it is truly an experimental feature. For example it does not honor any of the buildx/docker daemon configs for things like insecure registries so we do our best to fallback in those cases by passing --insecure flag which is not ideal but does not seem to be any way around it. In addition this: * ensures `CHALK_ID` is the same for all platform builds * ensures `METADATA_ID` is different for each platform by forcing `DOCKER_PLATFORM` to be part of the chalk * falls-back to docker build on docker push failures when buildx is being used (see code comment for context)
* updating tests/README.md how to setup docker locally * adding tests entrypoint to configure buildx for tests
miki725
force-pushed
the
ms/multiplatform
branch
from
25f9c5b
to
cd3defc
Compare
docker manifest create seems to be experimental command and it has quirks related to insecure registry whereas imagetools seems work much better out of the box so switching to it instead
viega
approved these changes
Oct 30, 2023
| @@ -500,7 +500,7 @@ proc addBuildCmdMetadataToMark(ctx: DockerInvocation) = | |||
| dict.setIfNeeded("DOCKER_CHALK_ADDED_TO_DOCKERFILE", | |||
| ctx.addedInstructions.join("\n")) | |||
|
|
|||
| proc prepareToBuild*(state: DockerInvocation) = | |||
| proc prepareToBuild(state: DockerInvocation) = | |||
There was a problem hiding this comment.
I don't like the Nim visibility rules; I tend to err on the side of making everything visible, because the errors can be obtuse sometimes when you actually need cross module visibility. So I wouldn't change these, but 🤷
There was a problem hiding this comment.
they werent used anywhere but I think we can keep things private just to avoid potential naming conflicts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
pending TODOs for PR:
docker pushhas different config thanbuildx. when using buildx, we should always--pushwithbuildxIssue
https://github.com/crashappsec/chalk-internal/issues/826
Description
This is a pretty crude approach which:
In addition this:
CHALK_IDis the same for all platform buildsMETADATA_IDis different for each platformby forcing
DOCKER_PLATFORMto be part of the chalkwhen buildx is being used (see code comment for context)
Testing
While tests are being worked on I used: