Add builder labels #1917
Add builder labels #1917
Conversation
caiges
force-pushed
the
add-builder-labels
branch
from
9894b5d
to
67ecdcc
Compare
|
Hi @caiges, thanks for opening the PR! I think it's a bit too early to merge it as there's no consensus yet in #1864. I suggest to wait a bit longer and see if this is something we could get into the OCI spec. This would prevent us from letting users depend on something that is likely to change in the future. |
|
Oh totally agree, I just wanted to throw out what the implementation & impact might look like. |
|
☔ The latest upstream changes (presumably f995696) made this pull request unmergeable. Please resolve the merge conflicts. |
cmd/buildah/commit.go
Outdated
| @@ -158,6 +158,10 @@ func commitCmd(c *cobra.Command, args []string, iopts commitInputOptions) error | |||
| } | |||
| } | |||
|
|
|||
| // Add builder information. | |||
| builder.SetLabel("org.opencontainers.image.builder.name", "buildah") | |||
| builder.SetLabel("org.opencontainers.image.builder.version", buildah.Version) | |||
There was a problem hiding this comment.
Based on @vbatts comment #1864 (comment), I think that we should not use the org.opencontainers.image prefix to prevent users from assuming that's a supported annotation of the OCI image spec.
How about a single io.buildah.version label? It's presence shows both, that is was built with buildah and with which version? @rhatdan @caiges @TomSweeneyRedHat @nalind WDYT?
There was a problem hiding this comment.
I concur on not using the org.opencontainers.image. Where's the "io" from in your suggestion @vrothberg ? My initial thought was "containers.image.*"
There was a problem hiding this comment.
I concur on not using the org.opencontainers.image. Where's the "io" from in your suggestion @vrothberg ? My initial thought was "containers.image.*"
The "io" comes from reversing the Buildah domain buildah.io -> io.buildah which might help consumers of an image find out what this label means. Maybe, just io.buildah as the key and have vX.Y.Z as the value?
There was a problem hiding this comment.
so it could be com.github.containers.buildah.version/commit too
There was a problem hiding this comment.
I like io.podman, nice and simple and easy to find.
There was a problem hiding this comment.
The spec's rules suggest the reverse-the-order-of-dns-labels method that's used for Java packages, D-Bus services, and others. This project doesn't "own" the com.github namespace prefix, and mustn't use the org.opencontainers prefix.
There was a problem hiding this comment.
I went with io.buildah for now since the podman/buildah relationship might be a little confusing if we were to use io.podman. I might be missing some context there though.
|
Needs a rebase. |
cmd/buildah/commit.go
Outdated
| @@ -158,6 +158,10 @@ func commitCmd(c *cobra.Command, args []string, iopts commitInputOptions) error | |||
| } | |||
| } | |||
|
|
|||
| // Add builder information. | |||
| builder.SetLabel("org.opencontainers.image.builder.name", "buildah") | |||
| builder.SetLabel("org.opencontainers.image.builder.version", buildah.Version) | |||
There was a problem hiding this comment.
Please make these annotation keys constants that we export, so that API consumers can refer to them without risking typos.
There was a problem hiding this comment.
That's a decent thought, I've exported the annotation key.
caiges
force-pushed
the
add-builder-labels
branch
from
8a57a44
to
5bbe760
Compare
|
LGTM assuming happy tests |
caiges
force-pushed
the
add-builder-labels
branch
from
5bbe760
to
f36fc76
Compare
Use io.podman.version and fix inspect tests Signed-off-by: caiges <[email protected]>
caiges
force-pushed
the
add-builder-labels
branch
from
f36fc76
to
9019805
Compare
|
@vrothberg @nalind @rhatdan PTAL |
|
@rh-atomic-bot retry |
|
📌 Commit 9019805 has been approved by |
|
☀️ Test successful - status-papr, status-travis |
PR 1966 has languished for three weeks without activity from submitter. In the interests of getting it online, I have taken it over and: - rebased - fixed several misunderstandings (bugs) noted in review feedback - fixed a few more I also slightly rewrote two tests (tag by id, commit with name) that were incomprehensible to me: unnecessary mount/umount and no actual testing of anything other than checking exit status. I believe the new code is closer to the intention of testing but please pay closer attention to those bits. Also: fixed the basic 'inspect' test. It looks like at some point in the last month containers#1917 added a version string to the buildah-inspect output. The test was fixed on master, but ypu's PR did not incorporate those fixes and the test was breaking. I took the liberty of cleaning up the entire test for readability and maintainability. Signed-off-by: Ed Santiago <[email protected]>
PR 1966 has languished for three weeks without activity from submitter. In the interests of getting it online, I have taken it over and: - rebased - fixed several misunderstandings (bugs) noted in review feedback - fixed a few more I also slightly rewrote two tests (tag by id, commit with name) that were incomprehensible to me: unnecessary mount/umount and no actual testing of anything other than checking exit status. I believe the new code is closer to the intention of testing but please pay closer attention to those bits. Also: fixed the basic 'inspect' test. It looks like at some point in the last month #1917 added a version string to the buildah-inspect output. The test was fixed on master, but ypu's PR did not incorporate those fixes and the test was breaking. I took the liberty of cleaning up the entire test for readability and maintainability. Signed-off-by: Ed Santiago <[email protected]> Closes: #2004 Approved by: rhatdan
This adds builder labels during a commit. Closes #1864
Signed-off-by: caiges [email protected]