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

Add metadata-file flag #605

Merged
merged 1 commit into from
Jun 30, 2021
Merged

Add metadata-file flag #605

merged 1 commit into from
Jun 30, 2021

Conversation

crazy-max
Copy link
Member

cc. @tonistiigi

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@crazy-max crazy-max marked this pull request as ready for review April 30, 2021 20:35
@crazy-max crazy-max force-pushed the bake-iidfile branch 2 times, most recently from 9d9e812 to e93dc9d Compare May 2, 2021 18:20
@tonistiigi
Copy link
Member

I think this would be better as metadata-file (eg. moby/buildkit#2095) and one per bake that lists all properties for all built targets instead of a file per target.

@AkihiroSuda thoughts?

@AkihiroSuda
Copy link
Collaborator

SGTM

@tonistiigi tonistiigi added this to the v0.6.0 milestone Jun 23, 2021
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.

^

@crazy-max
Copy link
Member Author

crazy-max commented Jun 24, 2021

Ok here is the current state on my side:

  • build command
    • --iidfile same behavior
    • --metadatafile write ExporterResponse (single target)
  • bake command
    • --iidfile remove this flag introduced here in favor of the new flag --metadatafile
    • --metadatafile write all build options and ExporterResponse for all targets. In this case I don't think we should handle a metadatafile target field.

@tonistiigi
Copy link
Member

@crazy-max SGTM. Having both iidfile and metadata-file in build may be a bit confusing atm but I guess it is fine. We could deprecate iidfile later or make it hidden.

https://github.com/moby/buildkit/pull/2095/files added flag with a dash. I think the dash is better.

@crazy-max
Copy link
Member Author

crazy-max commented Jun 24, 2021

@tonistiigi

Having both iidfile and metadata-file in build may be a bit confusing atm but I guess it is fine. We could deprecate iidfile later or make it hidden.

Agree about deprecation of iidfile.

https://github.com/moby/buildkit/pull/2095/files added flag with a dash. I think the dash is better.

Yes, I was wondering if we were keeping the same format as for iidfile but since we could deprecate it I will modify it to metadata-file.

@crazy-max crazy-max changed the title Add imageIDFile opt for bake Add metadata-file flag Jun 24, 2021
@crazy-max crazy-max force-pushed the bake-iidfile branch 3 times, most recently from affca3a to 6195760 Compare June 24, 2021 20:59
@crazy-max
Copy link
Member Author

crazy-max commented Jun 24, 2021

@tonistiigi Ok now metadata-file handles build/solve. Here is an example:

variable "DEFAULT_TAG" {
  default = "7zip: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",
    "linux/386",
    "linux/ppc64le",
    "linux/s390x"
  ]
}
docker buildx bake --metadata-file metadata.json image-local

metadata.json

{
  "metadata": {
    "build-opts": {
      "inputs": {
        "context": ".",
        "dockerfile": "Dockerfile"
      },
      "tags": [
        "7zip:local"
      ],
      "metadata-file": "./metadata.json"
    },
    "solve-opts": [
      {
        "containerimage.config.digest": "sha256:fe4387b1cf24e8b3f153b026e2843e8abddeda479b27fd6adf3ed7ad1f8eb67a",
        "containerimage.digest": "sha256:3eaf8fb892e653224be132bc26de3bfa82e4563750b5f1f6dd64ae25788c6247",
        "image.name": "docker.io/library/7zip:local"
      }
    ]
  }
}
docker buildx build --output type=docker --metadata-file ./metadata.json .

metadata.json

{
  "metadata": {
    "build-opts": {
      "inputs": {
        "context": "."
      },
      "metadata-file": "./metadata.json",
      "network-mode": "default"
    },
    "solve-opts": [
      {
        "containerimage.config.digest": "sha256:fe4387b1cf24e8b3f153b026e2843e8abddeda479b27fd6adf3ed7ad1f8eb67a",
        "containerimage.digest": "sha256:3eaf8fb892e653224be132bc26de3bfa82e4563750b5f1f6dd64ae25788c6247"
      }
    ]
  }
}

Let me know if it's the right direction.

@tonistiigi
Copy link
Member

I thought this was just going to contain the build result metadata. Why is that part called solve-opts? The rest is somewhat interesting but a different use-case and format is an internal representation that doesn't match anything else.

For the bake, the key should be image-local, not "metadata?"

Not related to this PR code but what is containerimage.config.digest for a multi-platform build.

@crazy-max
Copy link
Member Author

crazy-max commented Jun 25, 2021

@tonistiigi

I thought this was just going to contain the build result metadata. Why is that part called solve-opts?

Yes I was wondering about naming. Will call it metadata and set a "per-target" representation.

The rest is somewhat interesting but a different use-case and format is an internal representation that doesn't match anything else.

I wanted to see if it could be interesting in the future indeed. We can do this in a follow-up instead.

For the bake, the key should be image-local, not "metadata?"

Ok I can make a representation per-target

Not related to this PR code but what is containerimage.config.digest for a multi-platform build.

Will be empty:

docker buildx bake --set *.tags=crazymax/7zip:test --push --metadata-file ./metadata.json image-all
{
  "metadata": {
    "build-opts": {
      "inputs": {
        "context": ".",
        "dockerfile": "Dockerfile"
      },
      "tags": [
        "crazymax/7zip:test"
      ],
      "metadata-file": "./metadata.json",
      "platform": [
        {
          "architecture": "amd64",
          "os": "linux"
        },
        {
          "architecture": "arm",
          "os": "linux",
          "variant": "v6"
        },
        {
          "architecture": "arm",
          "os": "linux",
          "variant": "v7"
        },
        {
          "architecture": "arm64",
          "os": "linux"
        },
        {
          "architecture": "386",
          "os": "linux"
        },
        {
          "architecture": "ppc64le",
          "os": "linux"
        },
        {
          "architecture": "s390x",
          "os": "linux"
        }
      ]
    },
    "solve-opts": [
      {
        "containerimage.digest": "sha256:61ba426e49e207972d9ffaf6761cffec9fb3ae63dab76414cf250c9058370643",
        "image.name": "crazymax/7zip:test"
      }
    ]
  }
}

@crazy-max

This comment has been minimized.

@crazy-max crazy-max force-pushed the bake-iidfile branch 4 times, most recently from 2a4b27d to 55e8aa2 Compare June 25, 2021 16:38
@crazy-max crazy-max marked this pull request as draft June 25, 2021 16:40
@crazy-max crazy-max force-pushed the bake-iidfile branch 2 times, most recently from 893b749 to e17f173 Compare June 25, 2021 17:12
@crazy-max

This comment has been minimized.

@crazy-max

This comment has been minimized.

@crazy-max
Copy link
Member Author

@tonistiigi

build

docker buildx build --output type=docker --metadata-file ./metadata.json .
{
  "containerimage.config.digest": "sha256:d8b8b4f781520aeafedb5a88ff50fbb625cfebad87e392794f1e26a724a2f22a",
  "containerimage.digest": "sha256:868f04872380274dcf8528222e84dc66702394de80889e51c87a14126ea9ff6a"
}

bake

docker buildx bake --metadata-file metadata.json image-all image-local
{
  "image-all": {
    "containerimage.digest": "sha256:d9aeb67cb21c2b79da504342e3d30a6eb914b9d1b7df0c16b1947ee76587702c",
    "image.name": "crazymax/7zip:test"
  },
  "image-local": {
    "containerimage.config.digest": "sha256:d8b8b4f781520aeafedb5a88ff50fbb625cfebad87e392794f1e26a724a2f22a",
    "containerimage.digest": "sha256:868f04872380274dcf8528222e84dc66702394de80889e51c87a14126ea9ff6a",
    "image.name": "docker.io/library/7zip:local"
  }
}

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.

Small comments. Overall looks good 👍

build/build.go Outdated Show resolved Hide resolved
commands/bake.go Outdated Show resolved Hide resolved
docs/reference/buildx_build.md Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
build/build.go Outdated Show resolved Hide resolved
@crazy-max crazy-max force-pushed the bake-iidfile branch 2 times, most recently from 99ad3d3 to 506f0f7 Compare June 30, 2021 05:40
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.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.

Now that we import ioutils you can remove the duplicate function in store pkg in follow-up.

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.

None yet

3 participants