Skip to content
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: support removing multiple builders at once #2140

Merged
merged 1 commit into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 46 additions & 29 deletions commands/rm.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/docker/buildx/store"
"github.com/docker/buildx/store/storeutil"
"github.com/docker/buildx/util/cobrautil/completion"
"github.com/docker/cli/cli"
"github.com/docker/cli/cli/command"
"github.com/moby/buildkit/util/appcontext"
"github.com/pkg/errors"
Expand All @@ -18,7 +17,7 @@ import (
)

type rmOptions struct {
builder string
builders []string
keepState bool
keepDaemon bool
allInactive bool
Expand Down Expand Up @@ -46,50 +45,68 @@ func runRm(dockerCli command.Cli, in rmOptions) error {
return rmAllInactive(ctx, txn, dockerCli, in)
}

b, err := builder.New(dockerCli,
builder.WithName(in.builder),
builder.WithStore(txn),
builder.WithSkippedValidation(),
)
if err != nil {
return err
}
eg, _ := errgroup.WithContext(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that you are not passing the errgroup context to goroutines?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is intentional so even if one builder removal fails, it will continue with the removal of the remaining builders.

for _, name := range in.builders {
func(name string) {
eg.Go(func() (err error) {
defer func() {
if err == nil {
_, _ = fmt.Fprintf(dockerCli.Err(), "%s removed\n", name)
} else {
_, _ = fmt.Fprintf(dockerCli.Err(), "failed to remove %s: %v\n", name, err)
}
}()

nodes, err := b.LoadNodes(ctx)
if err != nil {
return err
}
b, err := builder.New(dockerCli,
builder.WithName(name),
builder.WithStore(txn),
builder.WithSkippedValidation(),
)
if err != nil {
return err
}

if cb := b.ContextName(); cb != "" {
return errors.Errorf("context builder cannot be removed, run `docker context rm %s` to remove this context", cb)
}
nodes, err := b.LoadNodes(ctx)
if err != nil {
return err
}

err1 := rm(ctx, nodes, in)
if err := txn.Remove(b.Name); err != nil {
return err
}
if err1 != nil {
return err1
if cb := b.ContextName(); cb != "" {
return errors.Errorf("context builder cannot be removed, run `docker context rm %s` to remove this context", cb)
}

err1 := rm(ctx, nodes, in)
if err := txn.Remove(b.Name); err != nil {
return err
}
if err1 != nil {
return err1
}

return nil
})
}(name)
}

_, _ = fmt.Fprintf(dockerCli.Err(), "%s removed\n", b.Name)
if err := eg.Wait(); err != nil {
return errors.New("failed to remove one or more builders")
}
return nil
}

func rmCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
var options rmOptions

cmd := &cobra.Command{
Use: "rm [NAME]",
Short: "Remove a builder instance",
Args: cli.RequiresMaxArgs(1),
Use: "rm [OPTIONS] [NAME] [NAME...]",
Short: "Remove one or more builder instances",
RunE: func(cmd *cobra.Command, args []string) error {
options.builder = rootOpts.builder
options.builders = []string{rootOpts.builder}
if len(args) > 0 {
if options.allInactive {
return errors.New("cannot specify builder name when --all-inactive is set")
}
options.builder = args[0]
options.builders = args
}
return runRm(dockerCli, options)
},
Expand Down
2 changes: 1 addition & 1 deletion docs/reference/buildx.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Extended build capabilities with BuildKit
| [`inspect`](buildx_inspect.md) | Inspect current builder instance |
| [`ls`](buildx_ls.md) | List builder instances |
| [`prune`](buildx_prune.md) | Remove build cache |
| [`rm`](buildx_rm.md) | Remove a builder instance |
| [`rm`](buildx_rm.md) | Remove one or more builder instances |
| [`stop`](buildx_stop.md) | Stop builder instance |
| [`use`](buildx_use.md) | Set the current builder instance |
| [`version`](buildx_version.md) | Show buildx version information |
Expand Down
4 changes: 2 additions & 2 deletions docs/reference/buildx_rm.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
# buildx rm

```text
docker buildx rm [NAME]
docker buildx rm [OPTIONS] [NAME] [NAME...]
```

<!---MARKER_GEN_START-->
Remove a builder instance
Remove one or more builder instances

### Options

Expand Down
1 change: 1 addition & 0 deletions tests/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func TestIntegration(t *testing.T) {
tests = append(tests, imagetoolsTests...)
tests = append(tests, versionTests...)
tests = append(tests, createTests...)
tests = append(tests, rmTests...)
testIntegration(t, tests...)
}

Expand Down
48 changes: 48 additions & 0 deletions tests/rm.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package tests

import (
"strings"
"testing"

"github.com/moby/buildkit/util/testutil/integration"
"github.com/stretchr/testify/require"
)

func rmCmd(sb integration.Sandbox, opts ...cmdOpt) (string, error) {
Expand All @@ -10,3 +14,47 @@ func rmCmd(sb integration.Sandbox, opts ...cmdOpt) (string, error) {
out, err := cmd.CombinedOutput()
return string(out), err
}

var rmTests = []func(t *testing.T, sb integration.Sandbox){
testRm,
testRmMulti,
}

func testRm(t *testing.T, sb integration.Sandbox) {
if sb.Name() != "docker-container" {
t.Skip("only testing for docker-container driver")
}

out, err := rmCmd(sb, withArgs("default"))
require.Error(t, err, out) // can't remove a docker builder

out, err = createCmd(sb, withArgs("--driver", "docker-container"))
require.NoError(t, err, out)
builderName := strings.TrimSpace(out)

out, err = inspectCmd(sb, withArgs(builderName, "--bootstrap"))
require.NoError(t, err, out)

out, err = rmCmd(sb, withArgs(builderName))
require.NoError(t, err, out)
}

func testRmMulti(t *testing.T, sb integration.Sandbox) {
if sb.Name() != "docker-container" {
t.Skip("only testing for docker-container driver")
}

var builderNames []string
for i := 0; i < 3; i++ {
out, err := createCmd(sb, withArgs("--driver", "docker-container"))
require.NoError(t, err, out)
builderName := strings.TrimSpace(out)

out, err = inspectCmd(sb, withArgs(builderName, "--bootstrap"))
require.NoError(t, err, out)
builderNames = append(builderNames, builderName)
}

out, err := rmCmd(sb, withArgs(builderNames...))
require.NoError(t, err, out)
}