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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

list-targets subaction #174

Merged
merged 2 commits into from Nov 16, 2023
Merged

list-targets subaction #174

merged 2 commits into from Nov 16, 2023

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Nov 9, 2023

subaction/matrix-targets/action.yml Outdated Show resolved Hide resolved
subaction/matrix-targets/action.yml Outdated Show resolved Hide resolved
@crazy-max crazy-max force-pushed the matrix-gen branch 2 times, most recently from 804e69f to 1d468bc Compare November 9, 2023 12:50
@crazy-max crazy-max marked this pull request as ready for review November 10, 2023 10:22
@tonistiigi
Copy link
Member

What's the difference of just setting the matrix value to docker buildx bake --print validate | jq .group.validate.targets ?

I think eventually we would want to do something that would auto-split based on platform for example and then merge the subimages built from matrix together into a single image. In that case, the current limited action could be confusing, or maybe it should be named differently to make the distinction clear.

@crazy-max
Copy link
Member Author

crazy-max commented Nov 13, 2023

What's the difference of just setting the matrix value to docker buildx bake --print validate | jq .group.validate.targets ?

This jq command does not support groups with matrix like:

{
  "group": {
    "default": {
      "targets": [
        "validate"
      ]
    },
    "lint": {
      "targets": [
        "lint-default",
        "lint-labs",
        "lint-nydus",
        "lint-yaml",
        "lint-proto"
      ]
    },
    "validate": {
      "targets": [
        "lint",
        "validate-vendor",
        "validate-doctoc",
        "validate-generated-files",
        "validate-shfmt",
        "validate-docs"
      ]
    }
  },
  ...

But jq -cr '.target | keys' would work, see moby/buildkit#4411 (comment).

I also prefer to avoid using third-party tools and shell but rely on the node runtime and GHA SDK so we can handle exceptions more easily. We can also display debug output so user can figure out how this was computed:

image

I think eventually we would want to do something that would auto-split based on platform for example and then merge the subimages built from matrix together into a single image. In that case, the current limited action could be confusing, or maybe it should be named differently to make the distinction clear.

Yes sounds good to handle matrix for a specific attribute so we can cover https://docs.docker.com/build/ci/github-actions/multi-platform/#with-bake use case as well.

I think matrix-targets is self-explanatory enough? For the platform case we could have a matrix-attribute subaction that would split based on the chosen attribute?:

variable "DEFAULT_TAG" {
  default = "app:local"
}

// Special target: https://github.com/docker/metadata-action#bake-definition
target "docker-metadata-action" {
  tags = ["${DEFAULT_TAG}"]
}

// Default target if none specified
group "default" {
  targets = ["image-local"]
}

target "image" {
  inherits = ["docker-metadata-action"]
}

target "image-local" {
  inherits = ["image"]
  output = ["type=docker"]
}

target "image-all" {
  inherits = ["image"]
  platforms = [
    "linux/amd64",
    "linux/arm/v6",
    "linux/arm/v7",
    "linux/arm64"
  ]
}
      -
        name: Attribute matrix
        id: matrix
        uses: docker/bake-action/subaction/matrix-attribute@v4
        with:
          target: image-all
          attribute: platforms

Or maybe you think of having a single subaction doing both?

@Nithos
Copy link
Contributor

Nithos commented Nov 13, 2023

This is great, thank you.
Yes this can be done with jq but like mentioned above it would be nice to have it covered directly within the action subsystem. This way any schema changes can be handled directly and not require updates to existing scripts etc..

Long term splitting across various platforms would be fantastic, not sure how that would work yet but it would be great if you could split it across specific runners so that ARM builds can be built using the appropriate runner as this will significantly reduce the build times.

@tonistiigi
Copy link
Member

? For the platform case we could have a matrix-attribute subaction that would split based on the chosen attribute?:

For the platforms split we would need a merge as well so it is more complicated.

Maybe list-targets is a better name. Matrix is already a Github term for how to use the value, so duplicated on the caller side and there might be other usages.

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
Copy link
Member Author

For the platforms split we would need a merge as well so it is more complicated.

Yes the platforms one is tricky. We need to save digests refs and then have another call to consume them. This should be feasible within a single action.

@crazy-max crazy-max changed the title matrix targets subaction list-targets subaction Nov 16, 2023
@tonistiigi tonistiigi merged commit db7848b into docker:master Nov 16, 2023
23 checks passed
@crazy-max crazy-max deleted the matrix-gen branch November 17, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Q: Proper bake usage withh groups
4 participants