-
Notifications
You must be signed in to change notification settings - Fork 471
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
rm: display removed builder and disallow removing context builders #1128
Conversation
7e73a0f
to
c3247f3
Compare
commands/rm.go
Outdated
if err := txn.Remove(ng.Name); err != nil { | ||
return err | ||
for _, cb := range ctxbuilders { | ||
if ng.Name == "default" && len(ng.Nodes) == 1 && ng.Nodes[0].Endpoint == cb.Name { |
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.
Not sure why there is a "default" name check in here. Shouldn't it check that ng.Driver == "docker"
? What does the "default" mean in here?
Didn't we already have an error suggesting to use docker context
somewhere or am I mixing it up with some other case.
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.
Not sure why there is a "default" name check in here.
This is how it's populated from store util for context instances:
buildx/store/storeutil/storeutil.go
Lines 127 to 134 in cdd391e
return &store.NodeGroup{ | |
Name: "default", | |
Nodes: []store.Node{ | |
{ | |
Name: "default", | |
Endpoint: name, | |
}, | |
}, |
Shouldn't it check that
ng.Driver == "docker"
?
I don't think it will work in standalone where docker is not available so a docker-container builder will be the "default": #1128 (comment)
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-container
is not the default ever, it always requires an instance to be created and therefore can always be removed.
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.
as discussed I switched to the cond on the driver name.
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
was looking at allowing removing multiple builders with a single invocation of
rm
command and also fixing removing timed out runners not in running state with--all-inactive
but before that small enhancements:atm if "default" is used and try to remove it, command exits without saying it cannot be removed. this PR fixes it and now error if we try to do so:
$ docker buildx use default $ docker buildx rm # or docker buildx rm default ERROR: default builder cannot be removed
we don't log anything if the builder has been effectively removed. It's useful in case we don't specify the builder so we know what has been removed:
$ docker buildx create --use nostalgic_gould $ docker buildx rm # or docker buildx rm nostalgic_gould nostalgic_gould removed
also adds another commit to uppercase error level to match the one of logrus.