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.

Sign up for GitHub

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

Jump to bottom

Create --append flag to add file to existing artifact using podman artifact add command #25179

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Create --append flag to add file to existing artifact using podman artifact add command #25179

wants to merge 2 commits into from
+258 −128

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Jan 31, 2025 •
edited
Loading

This PR creates a --append flag to add a file to an existing artifact using the podman artifact add command.

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

Does this PR introduce a user-facing change?

`podman artifact add` has new option: `--append`

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Jan 31, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 31, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Honny1
Once this PR has been reviewed and has the lgtm label, please assign ashley-cui for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@Honny1 Honny1 force-pushed the artifact-add-append branch 2 times, most recently from a23f466 to 1619f7f Compare January 31, 2025 13:29
@baude baude mentioned this pull request Jan 31, 2025
Merged
Luap99
Luap99 reviewed Jan 31, 2025
View reviewed changes
pkg/libartifact/artifact.go Outdated
Comment on lines 63 to 64
// TODO Here we have to assume only a single manifest for the artifact; this will
// need to evolve
Copy link
Member

Choose a reason for hiding this comment

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

FYI, talked with @baude about this I will open a PR later to just make it always use one manifest instead of the array as this simplifies a lot of things for all of us I think.
I can do this before your PR here or after, depending on how you prefer it.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can do it before. Please let me know when the PR is ready.

Copy link
Member

Choose a reason for hiding this comment

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

Sure I will work on it Monday

Copy link
Member

Choose a reason for hiding this comment

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

#25200

test/e2e/common_test.go Outdated
Comment on lines 1628 to 1636
func modifyArtifactFile(path string, newNumBytes int64) error {
dir, last := filepath.Split(path)
session := podmanTest.Podman([]string{"run", "-v", fmt.Sprintf("%s:/artifacts:z", dir), ALPINE, "dd", "if=/dev/urandom", fmt.Sprintf("of=%s", filepath.Join("/artifacts", last)), "bs=1b", fmt.Sprintf("count=%d", newNumBytes)})
session.WaitWithDefaultTimeout()
if session.ExitCode() != 0 {
return errors.New("unable to generate artifact file")
}
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems extremely wasteful to run container for this.
You can just create random byte sin go, i.e. math/rand and write them to the file.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2025
@Luap99
artifact: only allow single manifest
0666df3 
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]>
@Honny1 Honny1 force-pushed the artifact-add-append branch from 1619f7f to 1e16026 Compare February 3, 2025 13:48
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2025
@Honny1 Honny1 force-pushed the artifact-add-append branch from 1e16026 to cc2a56e Compare February 3, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luap99 Luap99 Luap99 left review comments

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Honny1 @Luap99 @openshift-merge-robot