Closes #9311 Handles container id/name collisions against daemon functionalities according to #8069 #9705
Conversation
|
@acbodine thanks for attacking this.. definitely a big set of changes to handle all the calls to daemon.Get(). The temporary patch is not required, right? Can that be squashed or why is it in the PR? Also, I was surprised to see more changes to truncindex than just the addition of a dup ID error; a lock and deferred unlock? Can you add some comments on why additional changes were necessary--mostly because I'm curious :) |
| // continue | ||
| // } | ||
| // } | ||
|
|
There was a problem hiding this comment.
Is this just the commented out version of the original code for 'inspect"? I think you can remove from the commit for clarity.
|
The general theme in this PR is that you added an error return result to many of the get methods for fetching containers from the daemon. However, you are only consuming these errors in your specific case. I think you could be consistent and always consume the errors and forget all the |
762aa7a to
e32e38e
Compare
|
@crosbymichael I totally agree with utilizing the errors returned from the daemon, and as you noted I have made some of those changes in the corresponding daemon methods that reference daemon.Get() The references that are returning an error message that doesn't quite match the error returned from the daemon are the ones that I left untouched. I'll set out on making these references more Go-like. |
e32e38e to
a296e57
Compare
|
@crosbymichael I think this should be all set now, utilizing the errors returned from daemon.Get |
|
@acbodine thanks, we will take another look soon |
| @@ -43,6 +44,11 @@ import ( | |||
| ) | |||
|
|
|||
| var ( | |||
| ErrNotFound = errors.New("No such container: %s") | |||
There was a problem hiding this comment.
This is used only in one place apart from tests and it is very confusing. Maybe just return this where it needed?
There was a problem hiding this comment.
This was meant to replace as many of the in place fmt.Errorf("No such container...") and I think it is necessary because this isn't the only error that can be returned from daemon.Get anymore.
There was a problem hiding this comment.
This is absolutely not necessary, because this is daemon.ErrNotFound. And making errors with %s is something which shouldn't be used. Now, instead of fmt.Errorf("No such container: %s", c) you need to write fmt.Sprintf(ErrNotFound.Error(), c), and it is only in two places, so you saved TWO symbols at the expense of having very strange line here. At least this should be just string, not error.
There was a problem hiding this comment.
Ah, I think I see what you mean. I'm making the changes. Getting rid of the static errors in favor of the in-place fmt.Errorf("...") and putting the verbose errors in the truncindex. As you suggested, will just return the errors from truncindex. Should be pushing these changes here soon.
c296eba to
008f690
Compare
|
@LK4D4 @crosbymichael is this ready to go now? |
| if container := daemon.Get(name); container != nil { | ||
|
|
||
| if container, err := daemon.Get(name); err != nil { | ||
| return job.Error(err) |
There was a problem hiding this comment.
you don't really need the else here if you are returning in the above if
008f690 to
acd16cd
Compare
|
@jfrazelle the else statements have been cleaned up |
| ErrNoID = errors.New("prefix can't be empty") | ||
| errDuplicateID = errors.New("multiple IDs were found") | ||
| ErrEmptyPrefix = errors.New("Prefix can't be empty") | ||
| ErrAmbiguousPrefix = errors.New("Too many containers found with provided prefix") |
There was a problem hiding this comment.
Because this is a generic package you should not have the word contianers in the error message. It mostly deals with strings as this is an index.
Can you change this to be something more generic for this package?
|
FYI @acbodine you will have to rebase due to the "log when truncIndex returns > 1" PR that was merged today. Given it had similar goals, except didn't go to the length of changing the |
f660dfe to
dd77682
Compare
|
@crosbymichael I adjusted the error messages to be more generic. |
| } | ||
|
|
||
| if err == truncindex.ErrDuplicateID { | ||
| log.Errorf("Short ID %s is ambiguous: please retry with more characters or use the full ID.\n", name) |
There was a problem hiding this comment.
The necessity for this log message isn't very clear to me. It's contents seem to be targeted for the cli user, but the user will never see this message unless they had access to, and are viewing the daemon logs. What's the purpose of this?
There was a problem hiding this comment.
Good point, looks like someone was just confused as it looks like a client side message, should be safe to remove.
There was a problem hiding this comment.
Removed the unnecessary daemon log.
b6a4f56 to
3acd52f
Compare
3acd52f to
4334893
Compare
|
ping @crosbymichael ... rebased |
|
|
||
| // prefix is an exact match to a full container Name | ||
| return containerByName, nil | ||
| } else if containerId != "" { |
There was a problem hiding this comment.
no need for else since you use return in the if part
389cb85 to
8fe451c
Compare
8fe451c to
3f50629
Compare
…functionalities according to moby#8069 Signed-off-by: Andrew C. Bodine <acbodine@us.ibm.com>
3f50629 to
d25a653
Compare
|
@crosbymichael @jfrazelle any more comments on this one? |
|
@acbodine Thank you. Nex time pls post "Closes *" info on second line of commit message. |
|
LGTM |
Changes truncindex package to properly report when multiple results were found with a given prefix
Modifies Daemon Get method according to the discussion in #8069
Fixes references to Daemon Get throughout. Mostly just ignoring the error message that is now returned, but replacing redundant logic where applicable. These clean-ups could have been more pervasive but I believe they will be easier to carry out once Docker has some notion of error lists defined to replace all of the hard-coded error messages.