SOURCE_DATE_EPOCH build arg injection fixes#1675
Conversation
| // Propagate SOURCE_DATE_EPOCH from the client env | ||
| if v := os.Getenv("SOURCE_DATE_EPOCH"); v != "" { | ||
| if _, ok := so.FrontendAttrs["build-arg:SOURCE_DATE_EPOCH"]; !ok { | ||
| so.FrontendAttrs["build-arg:SOURCE_DATE_EPOCH"] = v | ||
| } | ||
| } |
There was a problem hiding this comment.
For the case where buildx is used as lib (compose), it is not going to work anymore I guess?
There was a problem hiding this comment.
Mm you're right. I'm not quite sure if that's a bug or a feature though 🤔
I'm leaning towards it's probably best that when used as a library buildx should avoid reading from the environment as much as possible. We also have some minor precedent here - DOCKER_DEFAULT_PLATFROM.
Though I wonder about cases like BUILDX_NO_DEFAULT_ATTESTATIONS. On the one hand, it makes sense to propagate them, on the other, it could be confusing to the library consumer if the environment overrides the options that they have passed to Buildx.
IMO, in the future we should push all environment variables that affect the build solve request (so not including buildx driver env vars, etc) into the command layer, and then we should provide a utility method to parse environment variables and modify the build options from them:
buildx/controller/pb/controller.proto
Lines 47 to 73 in 4a73abf
Does that seem somewhat reasonable? If so, I'll add a TODO in the code - it's tricky to handle right now, since I think with the controller code, I'd expect we'd try and get library consumers of Buildx to use that API instead of using build.Build?
This allows the build package code to become more generic, and also ensures that when the environment variables are not propogated (in the case of the remote controller), that we can still correctly set SOURCE_DATE_EPOCH. Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
source-date-epoch-fixes
branch
from
fcb01a0 to
38b96da
Compare
Previously, when directly modifying the args map when reading targets, we could end up in a scenario where bake tests that compare arg maps would fail if SOURCE_DATE_EPOCH was set in the environment. This patch prevents this failure by setting the SOURCE_DATE_EPOCH at the command level (which isn't injected into tests as well), ensuring that we test correctly even when SOURCE_DATE_EPOCH is set in the environment. Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
source-date-epoch-fixes
branch
from
38b96da to
7805314
Compare
5 participants
This PR pushes all
SOURCE_DATE_EPOCHparsing into the command layer. This fixes two issues (in separate commits):buildcommand, we pull outSOURCE_DATE_EPOCHearly, and set it in the build-args - then in the case of the remote controller (which connects to a buildx server), we can still setSOURCE_DATE_EPOCHcorrectly.bakecommand, we pull outSOURCE_DATE_EPOCHearly, and set it as an override - this fixes docker-buildx: add developer-guy to maintainers list, upgrade docker-buildx to 0.10.4 NixOS/nixpkgs#221023 (comment), where ifSOURCE_DATE_EPOCHis set in the test environment (I think this is standard on Nix, though I'm not familiar) we generate additional build args. By splitting into a separate override in the bake command, tests don't end up accidentally reading the contents of the environment.