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

rkt/uuid: fix match when uuid is an empty string #2807

Merged
merged 1 commit into from Jun 21, 2016

Conversation

Projects
None yet
5 participants
@alepuccetti
Contributor

alepuccetti commented Jun 16, 2016

After the patch, if the uuid is an empty string then matchUUID(uuid string)
returns no matches instead of all the pods.
This is done to avoid rkt rm or rkt enter to delete or enter pods
without intending to.

Fixes #2806

@rktbot

This comment has been minimized.

Show comment
Hide comment
@rktbot

rktbot Jun 16, 2016

Can one of the admins verify this patch?

rktbot commented Jun 16, 2016

Can one of the admins verify this patch?

@iaguis

This comment has been minimized.

Show comment
Hide comment
@iaguis

iaguis Jun 16, 2016

Member

ok to test

Member

iaguis commented Jun 16, 2016

ok to test

@alban

This comment has been minimized.

Show comment
Hide comment
@alban

alban Jun 16, 2016

Member

How does it look like with this patch when users run rkt rm "" or rkt enter ""?

Member

alban commented Jun 16, 2016

How does it look like with this patch when users run rkt rm "" or rkt enter ""?

@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 16, 2016

Contributor

How does it look like with this patch when users run rkt rm "" or rkt enter ""?

rkt rm ""

rm: unable to resolve UUID: no matches found for ""
rm: failed to remove one or more pods

rkt enter ""
enter: problem retrieving pod: no matches found for ""
Contributor

alepuccetti commented Jun 16, 2016

How does it look like with this patch when users run rkt rm "" or rkt enter ""?

rkt rm ""

rm: unable to resolve UUID: no matches found for ""
rm: failed to remove one or more pods

rkt enter ""
enter: problem retrieving pod: no matches found for ""
@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 16, 2016

Contributor

Maybe the second line of the error message can be avoided when there are no matches.

Contributor

alepuccetti commented Jun 16, 2016

Maybe the second line of the error message can be avoided when there are no matches.

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Jun 16, 2016

Member

Two comments here:

  • this could benefit from using a ErrNoEmptyUUID
  • this is killing the (only?) way to act on all pods. I'm not sure if this feature has some real usecases. If so, should we introduce a special wildcard instead? Edit: nevermind, I later realized that the only caller of this explicitly expects a single result.
Member

lucab commented Jun 16, 2016

Two comments here:

  • this could benefit from using a ErrNoEmptyUUID
  • this is killing the (only?) way to act on all pods. I'm not sure if this feature has some real usecases. If so, should we introduce a special wildcard instead? Edit: nevermind, I later realized that the only caller of this explicitly expects a single result.
@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 16, 2016

Contributor

I agree on the wildcard, but before if you were running 'sudo rkt rm ""'
rkt was not going to delete all the pods anyway. It says 'ambiguous UUID'
this just catch a typing error.
That been said, I totally agree about the wildcard, but this should be a
RFE in a different PR. Which I can do it tomorrow.

On Thursday, June 16, 2016, Luca Bruno notifications@github.com wrote:

Two comments here:


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2807 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AGRc7ydEaG2LSqcP6ShTLJJQEyrA5fyFks5qMZCFgaJpZM4I3RWH
.

Contributor

alepuccetti commented Jun 16, 2016

I agree on the wildcard, but before if you were running 'sudo rkt rm ""'
rkt was not going to delete all the pods anyway. It says 'ambiguous UUID'
this just catch a typing error.
That been said, I totally agree about the wildcard, but this should be a
RFE in a different PR. Which I can do it tomorrow.

On Thursday, June 16, 2016, Luca Bruno notifications@github.com wrote:

Two comments here:


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2807 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AGRc7ydEaG2LSqcP6ShTLJJQEyrA5fyFks5qMZCFgaJpZM4I3RWH
.

@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 17, 2016

Contributor

Patch updated.

@lucab I will open a specific an issue for wildcard

Contributor

alepuccetti commented Jun 17, 2016

Patch updated.

@lucab I will open a specific an issue for wildcard

@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 17, 2016

Contributor

New error output

sudo rkt enter ""
enter: problem retrieving pod: UUID cannot be empty

sudo rkt rm ""
rm: unable to resolve UUID: UUID cannot be empty
rm: failed to remove one or more pods
Contributor

alepuccetti commented Jun 17, 2016

New error output

sudo rkt enter ""
enter: problem retrieving pod: UUID cannot be empty

sudo rkt rm ""
rm: unable to resolve UUID: UUID cannot be empty
rm: failed to remove one or more pods
Show outdated Hide outdated rkt/uuid.go Outdated
@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Jun 18, 2016

Member

One comment inline and some additional notes:

  • No need to specify the function in commit title, rkt/uuid should be enough.
  • Can you please also add a test for this?
Member

lucab commented Jun 18, 2016

One comment inline and some additional notes:

  • No need to specify the function in commit title, rkt/uuid should be enough.
  • Can you please also add a test for this?

@lucab lucab self-assigned this Jun 18, 2016

@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 20, 2016

Contributor

Patch updated. @lucab test added.

Contributor

alepuccetti commented Jun 20, 2016

Patch updated. @lucab test added.

Show outdated Hide outdated tests/rkt_rm_test.go Outdated
Alessandro Puccetti
rkt/uuid: fix match when uuid is an empty string
After the patch, if the uuid is an empty string then matchUUID(uuid string)
returns `UUID cannot be empty` error message instead of all the pods.
This is done to avoid `rkt rm` or `rkt enter` to delete or enter pods
without intending to.

Fixes #2806
@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 21, 2016

Contributor

patch updated. @lucab this should be cleaner

Contributor

alepuccetti commented Jun 21, 2016

patch updated. @lucab this should be cleaner

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Jun 21, 2016

Member

Yes, thanks! LGTM, mergeable once builders are done.

Member

lucab commented Jun 21, 2016

Yes, thanks! LGTM, mergeable once builders are done.

@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 21, 2016

Contributor

TestRmEmptyUUID passed, fedora-23 failure seems unrelated.

Contributor

alepuccetti commented Jun 21, 2016

TestRmEmptyUUID passed, fedora-23 failure seems unrelated.

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Jun 21, 2016

Member

@alepuccetti yes that's #2287. I'm merging this.

Member

lucab commented Jun 21, 2016

@alepuccetti yes that's #2287. I'm merging this.

@lucab lucab changed the title from rkt/matchUUID: fix match when uuid is an empty string to rkt/uuid: fix match when uuid is an empty string Jun 21, 2016

@lucab lucab merged commit 578e59a into rkt:master Jun 21, 2016

9 of 11 checks passed

Jenkins Build finished.
Details
fedora-23-flavor-coreos (i-4cfc8ff9) Failure :(
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
debian-8-flavor-coreos (i-effd8e5a) Success!
Details
debian-8-flavor-coreos (i-f6fd8e43) Success!
Details
fedora-22-flavor-coreos (i-9afc8f2f) Success!
Details
fedora-22-flavor-coreos (i-d0fc8f65) Success!
Details
fedora-23-flavor-coreos (i-a7fd8e12) Success!
Details
fedora-24-flavor-coreos (i-a6fd8e13) Success!
Details
fedora-24-flavor-coreos (i-dffc8f6a) Success!
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment