-
Notifications
You must be signed in to change notification settings - Fork 616
bake: add call methods support and printing #2556
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
Conversation
5d97a08 to
fe5d330
Compare
|
Should # Short form
target "default" {
call = "check"
}
# Long form
target "default" {
call = {
method = "check"
format = "json"
error = true # ignorestatus / BUILDKIT_DOCKERFILE_CHECK=error=true
skip = [ "NoEmptyContinuation", "StageNameCasing" ] # BUILDKIT_DOCKERFILE_CHECK=skip=StageNameCasing,NoEmptyContinuations
}
} |
Yes in the future we would like objects instead of csv values in HCL. This is tracked in #438 |
commands/bake.go
Outdated
| flags.BoolVar(&options.listTargets, "list-targets", false, "List available targets") | ||
| flags.BoolVar(&options.listVars, "list-variables", false, "List defined variables") |
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.
Should we open another PR for these flags? I don't think this is the same use case as call related to #1072
Also I think it should be a single flag --list defaulting to targets as value but can also be variables? I'm looking at https://github.com/casey/just#command-line-options
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.
Maybe we could consider using existing --print flag so we could pass --print=targets or --print=variables and --print would default to --print=definition?
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 don't think it is possible to use --print as both bool and string flag.
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.
Unless we have a clear use case, I would be keen not to overcomplicate things at this point. We can always make additional updates in the future
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.
list-targets and list-variables have been marked as experimental in the meantime.
|
Looks flaky |
|
Will open a follow-up when #2562 is merged to add cross linking from bake to build |
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.
@tonistiigi PTAL with my last commit
crazy-max
left a 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.
Hum now happens in https://github.com/docker/buildx/actions/runs/9781672942/job/27006277267?pr=2556#step:7:428
=== Failed
=== FAIL: tests TestIntegration/TestBakeCallCheckFlag/worker=docker-container (2.04s)
bake.go:1086:
Error Trace: /src/tests/bake.go:1086
/src/vendor/github.com/moby/buildkit/util/testutil/integration/run.go:96
/src/vendor/github.com/moby/buildkit/util/testutil/integration/run.go:211
Error: Received unexpected error:
invalid character '/' after top-level value
Test: TestIntegration/TestBakeCallCheckFlag/worker=docker-container
|
@crazy-max Something is printing to stdout on |
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
crazy-max
left a 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.
LGTM
opened #2575 to make sure we don't miss it before GA
|
Docs is missing for variable description field: 233b869#diff-dd01f2c9767e65fba215bfdbb1317fea48358318fa27866339a4dc3ef7757742R30 We should have an example in https://github.com/docker/buildx/blob/master/docs/bake-reference.md#variable similar to https://github.com/docker/buildx/blob/master/docs/bake-reference.md#targetdescription |
Define call method in the bake target
Set call/check on all active targets
The text output of
call=outlineshould be updated in follow-up to include bake definition and variables. Getting variables seems tricky and I think needs HCL parser updates.JSON output contains both Bake definition and the method result.
List all bake targets
List all bake variables