-
Notifications
You must be signed in to change notification settings - Fork 176
Add build parameters to docker app build command #697
Conversation
6d3e088
to
52d1328
Compare
Codecov Report
@@ Coverage Diff @@
## master #697 +/- ##
==========================================
- Coverage 72.08% 71.85% -0.23%
==========================================
Files 62 56 -6
Lines 3496 2931 -565
==========================================
- Hits 2520 2106 -414
+ Misses 653 552 -101
+ Partials 323 273 -50
Continue to review full report at Codecov.
|
52d1328
to
d946b87
Compare
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!
@glours can you rebase? 🦁 |
Is it possible to add some information to the PR description? |
internal/commands/build/types.go
Outdated
@@ -70,3 +76,30 @@ func transformBuildConfig(data interface{}) (interface{}, error) { | |||
return data, errors.Errorf("invalid type %T for service build", value) | |||
} | |||
} | |||
|
|||
func transformBuildArgsArray(array []string, sep string, allowNil bool) map[string]string { |
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.
Do we really need to pass the separator and allowNil
as this method is called only once with fixed values?
Why not something like mapBuildArgs
or buildArgsToMap
? Something that exposes the intention in the method name, transform
is not very clear.
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.
done
@eunomie PR description updated, sorry for that |
d946b87
to
df545da
Compare
internal/commands/build/types.go
Outdated
for _, value := range array { | ||
parts := strings.SplitN(value, "=", 2) | ||
key := parts[0] | ||
switch { |
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.
Why a switch here and not a
if len(parts) == 1 {
result[key] = ""
} else {
result[key] = parts[1]
}
that does the same thing but maybe more easy to read
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.
changed 😉
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'm fine with switch 😅
else
is bad, mkay?
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.
If else
is bad you can write:
result[key] = ""
if len(parts) != 1 {
result[key] = parts[1]
}
df545da
to
e3c893d
Compare
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
e3c893d
to
1ee6dd3
Compare
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
- What I did
Pass
--build-arg
parameter to buildx to build service images- How I did it
Add a
--build-arg
flagCheck a build arg isn't defined twice
Replace build arg defined in compose file & pass the list of args to buildx for generating service images
- How to verify it
run
docker --debug app build e2e/testdata/build/ --build-arg REPLACE_BY_BUILD_ARG=plop
check you see the following trace in the debug output in the
worker
section of thecom.docker.app.invocation-image
Find the
worker
image sha256 & then do adocker image inspect
with the given sha & make sure to haverun
docker app build e2e/testdata/build/single.dockerapp --build-arg REPLACE_BY_BUILD_ARG=plop --build-arg REPLACE_BY_BUILD_ARG
and check you have the following error message'--build-arg REPLACE_BY_BUILD_ARG' is defined twice
- A picture of a cute animal (not mandatory but encouraged)