add podman artifact extract #25238

Luap99 · 2025-02-05T18:13:22Z

Add a new command to extract the blob content of the artifact store to a local path.

Fixes https://issues.redhat.com/browse/RUN-2445

Does this PR introduce a user-facing change?

Added a new podman artifact extract command.

Add a new command to extract the blob content of the artifact store to a local path. Fixes https://issues.redhat.com/browse/RUN-2445 Signed-off-by: Paul Holzinger <[email protected]>

openshift-ci · 2025-02-05T18:13:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2025-02-05T18:14:34Z

Tests are still missing, will work on it tomorrow. Opening this to allow some early review.

cc @baude @Honny1 @mtrmac

packit-as-a-service · 2025-02-05T18:21:44Z

Ephemeral COPR build failed. @containers/packit-build please check.

baude · 2025-02-05T18:40:21Z

docs/source/markdown/podman-artifact-extract.1.md

+must be used to select only a single blob. If the file already exists it will be
+overwritten.
+
+If the target is an directory (it must exist) all blobs will be copied to the


Suggested change

If the target is an directory (it must exist) all blobs will be copied to the

If the target is an directory (it must exist), all blobs will be copied to the

baude · 2025-02-05T18:41:56Z

docs/source/markdown/podman-artifact-extract.1.md

+overwritten.
+
+If the target is an directory (it must exist) all blobs will be copied to the
+target directory. As target file name the value from `org.opencontainers.image.title`


Seems like a typo here maybe ... did you mean "a" or "the" target file ?

baude · 2025-02-05T18:42:13Z

docs/source/markdown/podman-artifact-extract.1.md

+
+If the target is an directory (it must exist) all blobs will be copied to the
+target directory. As target file name the value from `org.opencontainers.image.title`
+annotation is used. If the annotation is missing the target file name will be


Suggested change

annotation is used. If the annotation is missing the target file name will be

annotation is used. If the annotation is missing, the target file name will be

baude · 2025-02-05T18:42:31Z

docs/source/markdown/podman-artifact-extract.1.md

+target directory. As target file name the value from `org.opencontainers.image.title`
+annotation is used. If the annotation is missing the target file name will be
+the digest of the blob (with `:` replaced by `-` in the name).
+If the target file already exists in the directory it will be overwritten.


Suggested change

If the target file already exists in the directory it will be overwritten.

If the target file already exists in the directory, it will be overwritten.

mtrmac

A fairly brief skim; ACK overall, some more design questions.

Note the ⚠️ .

mtrmac · 2025-02-05T20:03:55Z

docs/source/markdown/podman-artifact-extract.1.md

+path is a file or does not exists the artifact must either consists of one blob
+or if it has multiple blobs (layers) then the **--digest** or **--title** option


s/consists/consist/

The (layers) explanation should probably happen with the first use of the word “blob”.

mtrmac · 2025-02-05T20:04:32Z

docs/source/markdown/podman-artifact-extract.1.md

+
+## DESCRIPTION
+
+Extract the blobs of an OCI artifact to a local file or directory. If the target


Consider a paragraph break after the first sentence; then there would be a paragraph for files, and a paragraph for directories.

mtrmac · 2025-02-05T20:06:04Z

docs/source/markdown/podman-artifact-extract.1.md

+overwritten.
+
+If the target is an directory (it must exist) all blobs will be copied to the
+target directory. As target file name the value from `org.opencontainers.image.title`


With --digest, the file name probably should be always digest-based, so that the user knows where to find the output.

(Same with --title, but that happens ~automatically.)

That makes the code actually simpler, I had the in my first version but then thought it would not be consistent. But yeah I guess you are right it makes sense to stick with the digest for users so they can predict the target name.

mtrmac · 2025-02-05T20:06:40Z

pkg/domain/entities/artifact.go

@@ -14,6 +14,13 @@ type ArtifactAddOptions struct {
 	ArtifactType string
 }

+type ArtifactExtractOptions struct {


Depending on what we decide about the option semantics, the comments here might need more detail (or to just refer to the man page for details)

mtrmac · 2025-02-05T20:08:07Z

pkg/domain/infra/abi/artifact.go

+	if err != nil {
+		return err
+	}
+	return artStore.Extract(ctx, name, target, (*types.ExtractOptions)(opts))


Can we avoid the cast? My reading of the Go spec is that this is actually safe (Go requires the two structs to have the same fields, in the same order), but to my old C eyes it looks like undefined behavior.

It would also make working with IDEs harder — tools to “find all references” of a field can’t trace how ArtifactExtractOptions.Digest becomes ExtractOptions.Digest.

(Disregard if this is a common pattern in Podman.)

I don't think it is common, I just found it shorter to write then assigning each filed individually. I can totally do that.

mtrmac · 2025-02-05T20:17:38Z

pkg/libartifact/store/store.go

+	return nil
+}
+
+func generateArtifactName(title string, digest digest.Digest) string {


Something like

Suggested change

func generateArtifactName(title string, digest digest.Digest) string {

func generateArtifactBlobName(title string, digest digest.Digest) string {

? It is not a name of the whole artifact.

mtrmac · 2025-02-05T20:18:48Z

pkg/libartifact/store/store.go

+	)
+	for _, l := range arty.Manifest.Layers {
+		if options.Digest == l.Digest.String() {
+			if len(digest.String()) > 0 {


(Absolutely non-blocking: IIRC digest != "" compiles to the same code, we can let the compiler turn that into a len check. Applies throughout.)

Fair but AFAICT we have no common style on these check empty string conditions in Podman. some do ==/!= "" and some do len() ==/!= 0 instead. I think I went consitently with len() here.

Also compare already existing

if len(dest) == 0 { return nil, ErrEmptyArtifactName }

mtrmac · 2025-02-05T20:20:31Z

pkg/libartifact/store/store.go

+		title  string
+	)
+	for _, l := range arty.Manifest.Layers {
+		if options.Digest == l.Digest.String() {


Non-blocking: This probably can be structured as if layerMatchesOptions(…) { duplicate check; set digest/title }). This works fine as is, and it is short enough that it is not that necessary.

mtrmac · 2025-02-05T20:22:16Z

pkg/libartifact/store/store.go

+		// on all platforms replace it with "-".
+		filename = strings.ReplaceAll(digest.String(), ":", "-")
+	}
+	return filename


⚠️

This must protect against .. in the path, maybe using filepath-securejoin, maybe outright rejecting paths with /, maybe something else.

Thanks, yes I totally forget about that. Let's reject /, we only support single files. Supporting actual paths with directories would means we have to mkdir them which means more code/tests, etc...
We can reconsider if users have a strong use case for them.

mtrmac · 2025-02-05T20:24:11Z

docs/source/markdown/podman-artifact-extract.1.md

+
+Extract the blobs of an OCI artifact to a local file or directory. If the target
+path is a file or does not exists the artifact must either consists of one blob
+or if it has multiple blobs (layers) then the **--digest** or **--title** option


Late thought: this does not allow extracting the config. I think that’s fine for the file-destination case, we can always add an option later.

For the directory-destination case, maybe we should establish the behavior right now. (It would not be a disaster not to handle that either, we can add an --include-config option later, but it seems worth thinking about.)

We do not allow setting the config either, AFAICT we also have {} as config. Extracting that always seems pointless because of that.

Note all man pages say:

## WARNING: Experimental command *This command is considered experimental and still in development. Inputs, options, and outputs are all subject to change.*

As such even if we merge it we can reconsider until we remove that warning so I rather to expand the scope of this PR to deal with configs.

add podman artifact extract

dab67a5

Add a new command to extract the blob content of the artifact store to a local path. Fixes https://issues.redhat.com/browse/RUN-2445 Signed-off-by: Paul Holzinger <[email protected]>

openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 5, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2025

baude reviewed Feb 5, 2025

View reviewed changes

mtrmac reviewed Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add podman artifact extract #25238

add podman artifact extract #25238

Luap99 commented Feb 5, 2025

openshift-ci bot commented Feb 5, 2025

Luap99 commented Feb 5, 2025

packit-as-a-service bot commented Feb 5, 2025

baude Feb 5, 2025

baude Feb 5, 2025

baude Feb 5, 2025

baude Feb 5, 2025

mtrmac left a comment

mtrmac Feb 5, 2025

mtrmac Feb 5, 2025

mtrmac Feb 5, 2025

Luap99 Feb 6, 2025

mtrmac Feb 5, 2025

mtrmac Feb 5, 2025

Luap99 Feb 6, 2025

mtrmac Feb 5, 2025

mtrmac Feb 5, 2025

Luap99 Feb 6, 2025

mtrmac Feb 5, 2025

mtrmac Feb 5, 2025

Luap99 Feb 6, 2025

mtrmac Feb 5, 2025

Luap99 Feb 6, 2025

	If the target is an directory (it must exist) all blobs will be copied to the
	If the target is an directory (it must exist), all blobs will be copied to the

	annotation is used. If the annotation is missing the target file name will be
	annotation is used. If the annotation is missing, the target file name will be

	If the target file already exists in the directory it will be overwritten.
	If the target file already exists in the directory, it will be overwritten.

		path is a file or does not exists the artifact must either consists of one blob
		or if it has multiple blobs (layers) then the --digest or --title option


		## DESCRIPTION

		Extract the blobs of an OCI artifact to a local file or directory. If the target

	func generateArtifactName(title string, digest digest.Digest) string {
	func generateArtifactBlobName(title string, digest digest.Digest) string {

add podman artifact extract #25238

Are you sure you want to change the base?

add podman artifact extract #25238

Conversation

Luap99 commented Feb 5, 2025

Does this PR introduce a user-facing change?

openshift-ci bot commented Feb 5, 2025

Luap99 commented Feb 5, 2025

packit-as-a-service bot commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment