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
Remove plugin root from filesystem. #25187
Conversation
@@ -37,6 +49,11 @@ func (s *DockerSuite) TestPluginBasicOps(c *check.C) { | |||
out, _, err = dockerCmdWithError("plugin", "remove", pNameWithTag) | |||
c.Assert(err, checker.IsNil) | |||
c.Assert(out, checker.Contains, pNameWithTag) | |||
|
|||
_, err = os.Stat(filepath.Join(dockerBasePath, "plugins", string(id))) | |||
if !os.IsNotExist(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check if err
is nil
here as well? the "fatal" may not be very useful if it's nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its fatal if err is anything other than IsNotExist, including nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, meant if the error message that's printed is very useful in that case (wondering)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err being nil doesn’t provide a very useful Log message, but what's important is c.Fatal marks the test as FAIL-ed. So this is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright
LGTM |
if _, err := tmpFile.Write([]byte(out)); err != nil { | ||
c.Fatal(err) | ||
} | ||
id, err := exec.Command("jq", ".Id", "--raw-output", tmpFile.Name()).CombinedOutput() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come we're using jq here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker plugin inspect
doesnt take a format, atm. Using jq is a stop-gap until inspect format is implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anusha-ragunathan can we add a comment here (a FIXME), to remember to update when plugin inspect
will take a format ? 👼
`docker plugin remove` didnt actually remove plugin from disk. Fix that. Signed-off-by: Anusha Ragunathan <anusha@docker.com>
bbf3cb2
to
5690730
Compare
LGTM |
docker plugin remove
didnt actually remove plugin from disk. Fix that.Signed-off-by: Anusha Ragunathan anusha@docker.com