Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement matrix for bake targets #1690

Merged
merged 6 commits into from
Apr 5, 2023
Merged

Implement matrix for bake targets #1690

merged 6 commits into from
Apr 5, 2023

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented Mar 21, 2023

馃敤 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):

group "validate" {
  targets = ["validate-lint", "validate-vendor", "validate-docs", "validate-authors"]
}

target "validate-lint" {
  dockerfile = "./hack/dockerfiles/lint.Dockerfile"
  output = ["type=cacheonly"]
}

target "validate-vendor" {
  dockerfile = "./hack/dockerfiles/vendor.Dockerfile"
  target = "validate"
  output = ["type=cacheonly"]
}


target "validate-docs" {
  dockerfile = "./hack/dockerfiles/docs.Dockerfile"
  target = "validate"
  output = ["type=cacheonly"]
}

target "validate-authors" {
  dockerfile = "./hack/dockerfiles/authors.Dockerfile"
  target = "validate"
  output = ["type=cacheonly"]
}

With this PR, the above example can be simplified to:

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 = "validate"
  output = ["type=cacheonly"]
}

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:

  • 鉁旓笍 Can we provide a shortcut to easily refer to all targets from a matrix without needing to manually specify them (see in the above example, where the ["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 (validate in this case) into a group that contains all the targets (so the validate group would be automatically created). This feels like the simplest option, but I'm not sure about the semantics of automatically creating groups 馃
  • Should we allow for a GitHub-matrix like syntax, to include or exclude specific matrix combinations? I think this would be useful, though I'm not sure of the exact syntax we'd want for this.
  • Should we support matrixes for groups as well? There's no reason why not, and adds consistency, but I struggle to see a concrete use case.

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 <me@jedevc.com>
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 <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc added kind/enhancement New feature or request area/bake labels Mar 21, 2023
@crazy-max
Copy link
Member

  • Can we provide a shortcut to easily refer to all targets from a matrix without needing to manually specify them (see in the above example, where the ["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 (validate in this case) into a group that contains all the targets (so the validate group would be automatically created). This feels like the simplest option, but I'm not sure about the semantics of automatically creating groups 馃

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"]
}

@crazy-max
Copy link
Member

With this PR, the above example can be simplified to:

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 = "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"]
}

@tonistiigi
Copy link
Member

@ciaranmcnulty @cpuguy83

@tonistiigi
Copy link
Member

Can we provide a shortcut to easily refer to all targets from a matrix without needing to manually specify them

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 <me@jedevc.com>
@jedevc
Copy link
Collaborator Author

jedevc commented Mar 22, 2023

Can we provide a shortcut to easily refer to all targets from a matrix without needing to manually specify them

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.

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"]
}
$ buildx bake --print validate
[+] Building 0.0s (0/0)                                                                                                                                           
{
  "group": {
    "default": {
      "targets": [
        "validate"
      ]
    },
    "validate": {
      "targets": [
        "validate-lint",
        "validate-vendor",
        "validate-docs",
        "validate-authors"
      ]
    }
  },
  "target": {
    "validate-authors": {
      "context": ".",
      "dockerfile": "./hack/dockerfiles/authors.Dockerfile",
      "target": "validate",
      "output": [
        "type=cacheonly"
      ]
    },
    "validate-docs": {
      "context": ".",
      "dockerfile": "./hack/dockerfiles/docs.Dockerfile",
      "target": "validate",
      "output": [
        "type=cacheonly"
      ]
    },
    "validate-lint": {
      "context": ".",
      "dockerfile": "./hack/dockerfiles/lint.Dockerfile",
      "target": "",
      "output": [
        "type=cacheonly"
      ]
    },
    "validate-vendor": {
      "context": ".",
      "dockerfile": "./hack/dockerfiles/vendor.Dockerfile",
      "target": "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.

@jedevc jedevc linked an issue Mar 22, 2023 that may be closed by this pull request
@jedevc jedevc changed the title Implement matrix for targets Implement matrix for bake targets Mar 22, 2023
bake/hcl_test.go Show resolved Hide resolved
bake/hcl_test.go Show resolved Hide resolved
bake/hcl_test.go Show resolved Hide resolved
bake/bake.go Show resolved Hide resolved
@ciaranmcnulty
Copy link

My main use case for this sort of thing is this sort of thing:

PHP_VERSIONS=81,82 FLAVOURS=debian,alpine docker buildx bake

It looks good for the basic cases 馃憤

@ciaranmcnulty
Copy link

As it's generating the product of all the lists in matrix, is there any way to remove an item from the resulting generated targets?

I'm thinking of cases where a particular combination isn't supported (e.g. latest version of a package isn't available on Debian)

@jedevc
Copy link
Collaborator Author

jedevc commented Mar 23, 2023

As it's generating the product of all the lists in matrix, is there any way to remove an item from the resulting generated targets?

Yeah, I think we probably will want that, see my second point:

Should we allow for a GitHub-matrix like syntax, to include or exclude specific matrix combinations? I think this would be useful, though I'm not sure of the exact syntax we'd want for this.

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 matrix.args is the block that actually contains the parameters, and then we could have matrix.include and matrix.exclude?

I'm not a huge fan of how GitHub mixes the include and exclude fields into the top-level, it feels unintuitive to me, but I could be convinced otherwise.

@ciaranmcnulty
Copy link

ciaranmcnulty commented Mar 23, 2023

It seems to me it's trivial to add to the matrix (which is slightly different to GHA's include) by simply defining some explicit additional targets that don't use this new mechanism

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc
Copy link
Collaborator Author

jedevc commented Mar 30, 2023

As another option instead of having a way of excludeing, we could potentially at some point in the future add another target option: skip. If skip is true, then we don't include it in the matrix, if it's false (the default), it is included.

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 matrix property of needing to nest them if possible - but let's hold off on adding anything to resolve this in the current PR.

doneB map[uint64]map[string]struct{}
}

type internalGetEvalContexts interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be public if we expect caller to implement it. And type checked with var _ WithEvalContexts = &Group{}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <me@jedevc.com>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but could you update docs as well

@jedevc
Copy link
Collaborator Author

jedevc commented Apr 5, 2023

We've moved the file definition into https://github.com/docker/docs, so I'll follow up there, cc @dvdksn.

@jedevc
Copy link
Collaborator Author

jedevc commented Apr 5, 2023

馃帀 Started the docs work in docker/docs#17041.

@jedevc jedevc merged commit f7d8bd2 into docker:master Apr 5, 2023
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bake kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: target pattern rule with bake Bake; placeholders or templating targets?
4 participants