rm: display removed builder and disallow removing context builders#1128
Conversation
crazy-max
force-pushed
the
rm
branch
2 times, most recently
from
7e73a0f
to
c3247f3
Compare
crazy-max
changed the title
crazy-max
changed the title
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.
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.
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
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.
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.
as discussed I switched to the cond on the driver name.
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
was looking at allowing removing multiple builders with a single invocation of
rmcommand and also fixing removing timed out runners not in running state with--all-inactivebut 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 removedwe 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 removedalso adds another commit to uppercase error level to match the one of logrus.