Skip to content

remove buildah requirement for the libpod image library - #1054

Closed
baude wants to merge 1 commit into
podman-container-tools:masterfrom
baude:removebuildahreq
Closed

remove buildah requirement for the libpod image library#1054
baude wants to merge 1 commit into
podman-container-tools:masterfrom
baude:removebuildahreq

Conversation

@baude

@baude baude commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

if we snip the requirement to use a buildah const in the libpod image library,
we can save something on the order of 85 vendored files in consumers of the
the library.

Signed-off-by: baude bbaude@redhat.com

@mheon

mheon commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

LGTM

Comment thread cmd/podman/commit.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is changing the value from ...manifest.v2... to ...manifest.v1...:

$ git grep V2S2MediaTypeManifest vendor | grep 'types\.go\|Dockerv2ImageManifest'
vendor/github.com/projectatomic/buildah/docker/types.go:const V2S2MediaTypeManifest = "application/vnd.docker.distribution.manifest.v2+json"
vendor/github.com/projectatomic/buildah/image.go:       Dockerv2ImageManifest = docker.V2S2MediaTypeManifest
$ git grep 'DockerV2Schema1MediaType =' vendor
vendor/github.com/containers/image/manifest/manifest.go:        DockerV2Schema1MediaType = "application/vnd.docker.distribution.manifest.v1+json"

I think you want to use DockerV2Schema2MediaType:

$ git grep 'application/vnd.docker.distribution.manifest.v2' vendor/github.com/containers/image/manifest            
vendor/github.com/containers/image/manifest/manifest.go:        DockerV2Schema2MediaType = "application/vnd.docker.distribution.manifest.v2+json"

(and similarly in the other file).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch ... fixed ... need caffeine

if we snip the requirement to use a buildah const in the libpod image library,
we can save something on the order of 85 vendored files in consumers of the
the library.

Signed-off-by: baude <bbaude@redhat.com>
Comment thread cmd/podman/commit.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you want manifest.DockerV2Schema2MediaType here.....

'''
image/manifest/manifest.go
// DockerV2Schema1MediaType MIME type represents Docker manifest schema 1
DockerV2Schema1MediaType = "application/vnd.docker.distribution.manifest.v1+json"
// DockerV2Schema2MediaType MIME type represents Docker manifest schema 2
DockerV2Schema2MediaType = "application/vnd.docker.distribution.manifest.v2+json"

Buildah side
image.go: Dockerv2ImageManifest = docker.V2S2MediaTypeManifest
./docker/types.go:const V2S2MediaTypeManifest = "application/vnd.docker.distribution.manifest.v2+json"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yup, @wking pointed this out

@baude
baude force-pushed the removebuildahreq branch from e068edf to 1ce187d Compare July 6, 2018 15:33
@TomSweeneyRedHat

Copy link
Copy Markdown
Contributor

Bah! @wking beat me. Change LGTM now. I just wonder if Buildah should also convert to using the constant from container/image too. @nalind any thoughts on that?

@mheon

mheon commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

@rh-atomic-bot r+

@rh-atomic-bot

Copy link
Copy Markdown

📌 Commit 1ce187d has been approved by mheon

@wking

wking commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

I just wonder if Buildah should also convert to using the constant from container/image too.

Personally, I don't have a problem with hard-coding the media type string wherever it's needed. Zero deps, and not all that many more characters in the Go source. But that is apparently not a popular approach, based on previous discussion in opencontainers/image-spec#320 and opencontainers/image-spec#414 ;).

@rh-atomic-bot

Copy link
Copy Markdown

⌛ Testing commit 1ce187d with merge 6092955...

@rh-atomic-bot

Copy link
Copy Markdown

☀️ Test successful - status-papr
Approved by: mheon
Pushing 6092955 to master...

@rhatdan

rhatdan commented Jul 7, 2018

Copy link
Copy Markdown
Contributor

@TomSweeneyRedHat Open a PR for buildah to use containers/image as well.

@baude
baude deleted the removebuildahreq branch December 22, 2019 19:08
@github-actions github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants