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

manifest add --artifact: handle multiple values #5728

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

nalind
Copy link
Member

@nalind nalind commented Sep 9, 2024

What type of PR is this?

/kind bug

What this PR does / why we need it:

Don't error out when manifest add --artifact is given multiple files, and add the test I should have added to check that.

How to verify it

New integration test!

Which issue(s) this PR fixes:

Fixes #5727.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

`buildah manifest add --artifact` no longer fails if it's invoked with multiple file arguments.  All of the files should be added to a single artifact manifest which should then be added to the image index being updated.

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 9, 2024
Copy link
Contributor

openshift-ci bot commented Sep 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nalind

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-ci openshift-ci bot added the approved label Sep 9, 2024
listImageSpec = args[0]
if listImageSpec == "" {
return fmt.Errorf(`Invalid image name "%s"`, args[0])
}

This comment was marked as outdated.

@lzap
Copy link

lzap commented Sep 9, 2024

Thanks for looking into that, I was writing similar PR but you were faster :-)

Once this gets merged, is there a "development" container tag we can use until this gets into stable?

Edit: I guess that would be: https://quay.io/repository/buildah/testing

Edit 2: Hmmm comes from Fedora testing I guess, hopefully it is a nightly build in there.

Edit 3: Okay it is couple of days old: 102:1.37.0-1.20240906151913564690.main.102.g695a29d9b.fc40

@nalind nalind force-pushed the multiple-artifacts branch from 106fff5 to e7e4727 Compare September 9, 2024 15:03
artifactSpec = args[1:]
} else {
return errors.New("Too many arguments: expected list and image add to list")
}
}
Copy link

Choose a reason for hiding this comment

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

This could have been just:

if len(args) < 2 {
  return errors.New("At least a list image and an image or artifact to add must be specified")
}

// the rest of the code - three idents saved

@nalind
Copy link
Member Author

nalind commented Sep 9, 2024

Thanks for looking into that, I was writing similar PR but you were faster :-)

Once this gets merged, is there a "development" container tag we can use until this gets into stable?

The quay.io/buildah/upstream image should pick up changes before too long. The git commit information is stored in the release field of the RPM that's built from upstream instead of in the binary itself, so it's not immediately obvious from the image's metadata.

Copy link

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

@lzap
Copy link

lzap commented Sep 11, 2024

Test are failing, these looks like relevant failures: Error: parsing reference "": repository name must have at least one component

Don't error out when `manifest add --artifact` is given multiple files,
and add a test which should have checked that.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Sep 11, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 11, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 49406e1 into containers:main Sep 11, 2024
32 checks passed
@nalind nalind deleted the multiple-artifacts branch September 11, 2024 18:44
@nalind
Copy link
Member Author

nalind commented Sep 11, 2024

@TomSweeneyRedHat any reason I shouldn't try to cherry-pick this for release-1.37?

@lzap
Copy link

lzap commented Sep 12, 2024

Thanks for taking a look @nalind !

@nalind
Copy link
Member Author

nalind commented Sep 16, 2024

/cherry-pick release-1.37

@openshift-cherrypick-robot

@nalind: new pull request created: #5739

In response to this:

/cherry-pick release-1.37

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.

@TomSweeneyRedHat
Copy link
Member

@nalind I'm late here, but the Buildah v1.37 branch is supposed to be closed unless there's a Jira card associated for the fix, or the fix is localized to Test code.

@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Dec 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved kind/bug Categorizes issue or PR as related to a bug. lgtm locked - please file new issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding manifest does not work with multiple files
5 participants