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

Handle single character images #7158

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 30, 2020

Currently you can only specify multiple character for image names
when executing podman-remote commit

podman-remote commit a b
Will complete, but will save the image without a name.

podman-remote commit a bb
Works.

This PR fixes and now returns an error if the user doees not specify an
image name to commit to.

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Jul 30, 2020

fixes: #7114
@edsantiago PTAL

@mheon
Copy link
Member

mheon commented Jul 30, 2020

Can you add a test?

@edsantiago
Copy link
Member

LGTM, code seems to work. Wish I'd replied sooner, I would've just suggested piggybacking on an existing commit test (changing a long name to a single-letter one) instead of adding more time to the CI run. But this works too.

@giuseppe
Copy link
Member

LGTM

// I know mitr hates this ... but doing for now
if len(query.Repo) > 1 {
destImage = fmt.Sprintf("%s:%s", query.Repo, tag)
if len(query.Repo) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

I'm always leery about " equals some number" could you convert to < 1?

@edsantiago
Copy link
Member

@rhatdan CI failures look real, in API tests:

not ok 154 [20-containers] POST libpod/commit?container=myfoo [] : status
#  expected: 200
#    actual: 500

I have to guess that your change somehow broke a corner case in APIv2.

@edsantiago
Copy link
Member

edsantiago commented Jul 30, 2020

Yep, looks like 'podman commit containername`, with no imagename, is perfectly valid:

$ ./bin/podman commit --help
   podman commit [flags] CONTAINER [IMAGE]
$ ./bin/podman run --name a alpine true
$ ./bin/podman commit a
Getting image source signatures
Copying blob 50644c29ef5a skipped: already exists
Copying blob e624f929ee9f done
Copying config d8a029cf84 done
Writing manifest to image destination
Storing signatures
d8a029cf8407bc6406697214a1bbdfe8729a845e99323f5bfd590e6ea73c974b

...but with this PR, that usage is no longer being allowed.

Currently you can only specify multiple character for image names
when executing podman-remote commit

podman-remote commit a b
Will complete, but will save the image without a name.

podman-remote commit a bb
Works.

This PR fixes and now returns an error if the user doees not specify an
image name to commit to.

Signed-off-by: Daniel J Walsh <[email protected]>
@edsantiago
Copy link
Member

@cevich we're seeing a lot of these, on many different PRs and over a long period of time (all last week):

Uploading 3 artifacts for /var/tmp/go/src/github.com/containers/podman/*.log.html
Uploaded /var/tmp/go/src/github.com/containers/podman/apiv2_test.log.html
Failed to upload artifact file /var/tmp/go/src/github.com/containers/podman/integration_test.log.html: EOF

Does it look familiar? Is there anything you can think of that can make this more robust?

@edsantiago
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2020
@openshift-merge-robot openshift-merge-robot merged commit 41358f5 into containers:master Aug 3, 2020
@cevich
Copy link
Member

cevich commented Aug 3, 2020

Does it look familiar? Is there anything you can think of that can make this more robust?

Yes, I've heard of this, and from multiple containers repositories (i.e. it's not just a problem in one google-project or github project or cirrus-ci configuration). Since yours represents "even more repeated complaints" now, I best bring it to the attention of CIrrus support. That's not a guarantee that can/will do anything about it, but maybe they can help find where the problem comes from (so we can avoid it somehow).

@edsantiago
Copy link
Member

@cevich here are three just from this PR, all demonstrating the same three-line failure:

https://cirrus-ci.com/task/5071485364600832
https://cirrus-ci.com/task/5339766201778176
https://cirrus-ci.com/task/5621241178488832

Here's one with a zero-line failure:

https://cirrus-ci.com/task/4508535411179520

@cevich
Copy link
Member

cevich commented Aug 3, 2020

@edsantiago thanks, I'll get those right over to them.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants