Implement matrix for bake targets#1690
Conversation
This patch allows high level clients to define an EvalContext method which can derive a new context given a block and the base parent context. This allows users of the package to intercept evaluation before it begins, and define additional variables and functions that are bound to a single block. Signed-off-by: Justin Chadwell <[email protected]>
Previously, the name property could not be set in the body of a bake target and could only be set for a label. This patch allows the body to override the values of label fields, though the default is still the label. Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: Justin Chadwell <[email protected]>
I don't think it's related to the matrix proposal but #1312 to use target pattern rules with stem: group "validate" {
targets = ["validate-lint", "validate-vendor", "validate-docs", "validate-authors"]
}
target "validate-%" {
dockerfile = "./hack/dockerfiles/${*}.Dockerfile"
target = "${*}" != "lint" ? "validate" : ""
output = ["type=cacheonly"]
} |
Should be: group "validate" {
targets = ["validate-lint", "validate-vendor", "validate-docs", "validate-authors"]
}
target "validate" {
matrix = {
tgt = ["lint", "vendor", "docs", "authors"]
}
name = "validate-${tgt}"
dockerfile = "./hack/dockerfiles/${tgt}.Dockerfile"
target = "${tgt}" != "lint" ? "validate" : ""
output = ["type=cacheonly"]
} |
Yes, please. There should not be a need to define an extra group with the same matrix values repeated as targets. When calling a target that defines matrix, all matrix combinations are built. |
Signed-off-by: Justin Chadwell <[email protected]>
Done in 0806870. For the input: # no separate group necessary!
target "validate" {
matrix = {
tgt = ["lint", "vendor", "docs", "authors"]
}
name = "validate-${tgt}"
dockerfile = "./hack/dockerfiles/${tgt}.Dockerfile"
target = "${tgt}" != "lint" ? "validate" : ""
output = ["type=cacheonly"]
}If the group is defined separately, it will be correctly merged with the previous target - the underlying representation is a little, but I think it's what the user expects. |
|
My main use case for this sort of thing is this sort of thing:
It looks good for the basic cases 👍 |
|
As it's generating the product of all the lists in I'm thinking of cases where a particular combination isn't supported (e.g. latest version of a package isn't available on Debian) |
Yeah, I think we probably will want that, see my second point:
I'm not quite sure on syntax, but it's also definitely a thing for a follow-up PR. The easiest suggestion that springs to mind: target "validate" {
matrix = {
tgt = ["lint", "vendor", "docs", "authors"]
}
matrix-include = [
{tgt = "another-option"} # this option is added to the list of matrix combinations
]
matrix-exclude = [
{tgt = "authors"} # this option is removed from the list of matrix combinations
]
name = "validate-${tgt}"
dockerfile = "./hack/dockerfiles/${tgt}.Dockerfile"
target = "${tgt}" != "lint" ? "validate" : ""
output = ["type=cacheonly"]
}It feels not great though, maybe it would make sense to group all of them under one single property, so that I'm not a huge fan of how GitHub mixes the |
|
It seems to me it's trivial to add to the matrix (which is slightly different to GHA's |
Signed-off-by: Justin Chadwell <[email protected]>
|
As another option instead of having a way of For example: target "test" {
matrix = {
driver = ["docker", "docker-container", "kubernetes", "remote"]
test = ["a", "b", "c"]
}
name = "test-${driver}-${test}"
skip = driver == "kubernetes" && test == "c"
}I'd prefer something like this, instead of another |
| doneB map[uint64]map[string]struct{} | ||
| } | ||
|
|
||
| type internalGetEvalContexts interface { |
There was a problem hiding this comment.
This should be public if we expect caller to implement it. And type checked with var _ WithEvalContexts = &Group{}
There was a problem hiding this comment.
Done. I've type checked for Group + Target for both the interfaces.
This adds the following constraints to the new features: - Explicit renaming with the `name` property is *only* permitted when used with the `matrix` property. - Group does not support either `name` or `matrix` (we may choose to relax this constraint over time). - All generated names must be unique. Signed-off-by: Justin Chadwell <[email protected]>
tonistiigi
left a comment
There was a problem hiding this comment.
LGTM but could you update docs as well
|
We've moved the file definition into https://github.com/docker/docs, so I'll follow up there, cc @dvdksn. |
|
🎉 Started the docs work in docker/docs#17041. |
🔨 Fixes #1150 and #1312
This PR implements the proposal from #1150 (comment), setting a matrix property on targets which can be used to easily parameterize builds such as #1312 (comment):
With this PR, the above example can be simplified to:
See the added tests for more information on the functionality - I'll add some docs once it's clear there's some consensus on the design + implementation.
There's still some work to be done around the matrix feature, though I'd be happy to do them as follow-ups, I think there's enough functionality to review in this PR. Specifically, we should consider:
["lint", "vendor", "docs", "authors"]is effectively duplicated, once in the matrix definition and once in the group definition)? We could implicitly turn the old label from the target (validatein this case) into a group that contains all the targets (so thevalidategroup would be automatically created). This feels like the simplest option, but I'm not sure about the semantics of automatically creating groups 🤔includeorexcludespecific matrix combinations? I think this would be useful, though I'm not sure of the exact syntax we'd want for this.