Skip to content

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Oct 6, 2025

Does this PR introduce a user-facing change?

Users of podman artifact add can override the org.opencontainers.image.title annotation.

@rhatdan
Copy link
Member Author

rhatdan commented Oct 6, 2025

@Luap99 PTAL

@ninja-quokka
Copy link
Collaborator

An issue this introduces is when using podman artifact add --annotation [1] the annotation is added to each file/blob passed in the call.

So ./bin/podman artifact add --annotation=org.opencontainers.image.title=testname multitest README.md MAINTAINERS.md results in an artifact with two files with the image title testname

So we need to do something like block setting the org.opencontainers.image.title if multiple artifacts are being added in one call or maybe better yet fail if the artifact already has file/blob with the same image.title as this will cover the situation of adding a file/blob to an existing artifact with podman artifact --append

@baude WDYT

[1] https://docs.podman.io/en/v5.6.2/markdown/podman-artifact-add.1.html#annotation-annotation-value

@rhatdan
Copy link
Member Author

rhatdan commented Oct 6, 2025

The same issue exists if you do the same name multiple times.

./bin/podman artifact add --annotation=org.opencontainers.image.title=testname multitest README.md
./bin/podman artifact add --annotation=org.opencontainers.image.title=testname multitest Maintainers.md

So we can either check to see if the annotation is already defined, or just leave it to the users to deal with it. IE this is legal theoretically in the spec, so if users add it then they have to deal with it when the attempt to mount the object.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

looks reasonable, as we discussed in the community meeting we should generate an error if the same name is set for more than one file.

title := RandomString(12)
annotation3 := fmt.Sprintf("org.opencontainers.image.title=%s", title)
podmanTest.PodmanExitCleanly("artifact", "add", "--annotation", annotation3, "foobar", artifact1File)
Expect(a.Manifest.Layers[0].Annotations["org.opencontainers.image.title"]).To(Equal(title))
Copy link
Member

Choose a reason for hiding this comment

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

that is not checking the right thing, you need to inspect the artifact you just created with podmanTest.InspectArtifact("foobar") and use that return value to check the annotation

@TomSweeneyRedHat
Copy link
Member

Code LGTM
Tests are bubbling, I restarted, but I'm not 100% convinced they're flakes.

@TomSweeneyRedHat
Copy link
Member

And Happy Green Test buttons.
LGTM

@rhatdan
Copy link
Member Author

rhatdan commented Oct 7, 2025

This is not ready yet, since we decided at the meeting today to block users from using the same title multiple times within an artifact.

@baude
Copy link
Member

baude commented Oct 8, 2025

This is not ready yet, since we decided at the meeting today to block users from using the same title multiple times within an artifact.

Would you mind pinging us when you have this in a state you want a review done?

@rhatdan
Copy link
Member Author

rhatdan commented Oct 15, 2025

@baude @Luap99 @mheon This is ready to go, I believe.

@rhatdan rhatdan added the 5.7 label Oct 16, 2025
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

I just merged containers/container-libs#392 so all the libartifact changes must now go there instead unfortunately

@Luap99
Copy link
Member

Luap99 commented Oct 16, 2025

Ok since the contianer-libs merged you should be able to revendor that here

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2025
Copy link
Collaborator

@ninja-quokka ninja-quokka left a comment

Choose a reason for hiding this comment

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

/LGTM

Thanks Dan!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2025
Copy link
Contributor

openshift-ci bot commented Oct 17, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, ninja-quokka, rhatdan

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:

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

@openshift-merge-bot openshift-merge-bot bot merged commit ac0c724 into containers:main Oct 17, 2025
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5.7 approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. machine release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants