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

artifact: only allow single manifest #25200

Merged

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Feb 3, 2025

Allowing for multiple manifest per artifact just makes the code and cli design harder to work with it. It is not clear how mounting, extracting or edit on a multi manifest artifact should have worked.

A single manifest should make the code much easier to work with.

Does this PR introduce a user-facing change?

None

@Luap99
Copy link
Member Author

Luap99 commented Feb 3, 2025

cc @baude @Honny1 @mtrmac

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

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

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

LGTM, I just have a minor, non-blocking question.

pkg/libartifact/artifact.go Show resolved Hide resolved
Copy link
Contributor

openshift-ci bot commented Feb 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, 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:

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

@mheon
Copy link
Member

mheon commented Feb 3, 2025

Code LGTM but I don't want to merge without comments from @baude and/or @mtrmac

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

A quick take if it helps: I have no concern with this PR from c/image perspective.

I am not currently familiar with the Podman artifact code enough to have much of an opinion on the API design or other aspects of these subpackages.

pkg/libartifact/store/store.go Outdated Show resolved Hide resolved
pkg/libartifact/artifact.go Show resolved Hide resolved
Allowing for multiple manifest per artifact just makes the code and cli
design harder to work with it. It is not clear how mounting, extracting
or edit on a multi manifest artifact should have worked.

A single manifest should make the code much easier to work with.

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99 Luap99 force-pushed the artifact-single-manifest branch from 0666df3 to 6c06577 Compare February 4, 2025 11:17
@Luap99
Copy link
Member Author

Luap99 commented Feb 4, 2025

@baude @mtrmac PTAL again

@baude
Copy link
Member

baude commented Feb 4, 2025

@Honny1 i think i am stating the obvious but this will likely impact #25179

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 16a72c8 into containers:main Feb 4, 2025
79 checks passed
@Luap99 Luap99 deleted the artifact-single-manifest branch February 4, 2025 16:40
@baude
Copy link
Member

baude commented Feb 4, 2025

/cherry-pick v5.4

@openshift-cherrypick-robot
Copy link
Collaborator

@baude: #25200 failed to apply on top of branch "v5.4":

Applying: artifact: only allow single manifest
Using index info to reconstruct a base tree...
M	pkg/libartifact/store/store.go
M	test/e2e/artifact_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/artifact_test.go
CONFLICT (content): Merge conflict in test/e2e/artifact_test.go
Auto-merging pkg/libartifact/store/store.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 artifact: only allow single manifest

In response to this:

/cherry-pick v5.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants