tools: Remove BicepCli interface (#4116)

When we started to depend on external tools, we used a pattern of
defining an interface to represent the capabilities of the tool which
the rest of the system dependend on.  The thought was having this
interface indirection would help us with unit-testing, because we
could mock different parts of the tool directly instead of depending
on mocks via the external command runner.

In practice, we never did this, when we test things, we just mock at
the `exec.CommandRunner` layer or use something like the recorder to
capture all the external tool interactions and replay them later.

Since we don't have multiple implementations of the interface, let's
just remove it and write things in the more direct style as we haven
been doing more recently where we don't have the intermediate
interface.

With this change, `bicep.BicepCli` (an interface) is now replaced with
`*bicep.Cli` which is logically what was returned in the past anyway,
but behind the guise of the interface).

Author: ellismg

cli/azd/internal/scaffold/scaffold_test.go

@@ -149,7 +149,7 @@ func TestExecInfra(t *testing.T) {
 			}
 
 			ctx := context.Background()
-			cli, err := bicep.NewBicepCli(ctx, mockinput.NewMockConsole(), exec.NewCommandRunner(nil))
+			cli, err := bicep.NewCli(ctx, mockinput.NewMockConsole(), exec.NewCommandRunner(nil))
 			require.NoError(t, err)
 
 			res, err := cli.Build(ctx, filepath.Join(dir, "main.bicep"))

cli/azd/pkg/azd/default.go

@@ -46,7 +46,7 @@ func (p *DefaultPlatform) IsEnabled() bool {
 func (p *DefaultPlatform) ConfigureContainer(container *ioc.NestedContainer) error {
 	// Tools
 	container.MustRegisterSingleton(terraform.NewTerraformCli)
-	container.MustRegisterSingleton(bicep.NewBicepCli)
+	container.MustRegisterSingleton(bicep.NewCli)
 
 	// Provisioning Providers
 	provisionProviderMap := map[provisioning.ProviderKind]any{

cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go

@@ -67,7 +67,7 @@ type BicepProvider struct {
 	projectPath           string
 	options               Options
 	console               input.Console
-	bicepCli              bicep.BicepCli
+	bicepCli              *bicep.Cli
 	azCli                 azcli.AzCli
 	deploymentsService    azapi.Deployments
 	deploymentOperations  azapi.DeploymentOperations
@@ -2194,7 +2194,7 @@ func isValueAssignableToParameterType(paramType ParameterType, value any) bool {
 
 // NewBicepProvider creates a new instance of a Bicep Infra provider
 func NewBicepProvider(
-	bicepCli bicep.BicepCli,
+	bicepCli *bicep.Cli,
 	azCli azcli.AzCli,
 	deploymentsService azapi.Deployments,
 	deploymentOperations azapi.DeploymentOperations,

cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go

@@ -86,7 +86,7 @@ func TestBicepPlanPrompt(t *testing.T) {
 	mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool {
 		return strings.Contains(args.Cmd, "bicep") && args.Args[0] == "--version"
 	}).Respond(exec.RunResult{
-		Stdout: fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.BicepVersion.String()),
+		Stdout: fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.Version.String()),
 		Stderr: "",
 	})
 
@@ -201,7 +201,7 @@ func TestPlanForResourceGroup(t *testing.T) {
 	mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool {
 		return strings.Contains(args.Cmd, "bicep") && strings.Contains(command, "--version")
 	}).RespondFn(func(args exec.RunArgs) (exec.RunResult, error) {
-		return exec.NewRunResult(0, fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.BicepVersion), ""), nil
+		return exec.NewRunResult(0, fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.Version), ""), nil
 	})
 
 	// Have `bicep build` return a ARM template that targets a resource group.
@@ -355,7 +355,7 @@ func createBicepProvider(t *testing.T, mockContext *mocks.MockContext) *BicepPro
 	envManager := &mockenv.MockEnvManager{}
 	envManager.On("Save", mock.Anything, mock.Anything).Return(nil)
 
-	bicepCli, err := bicep.NewBicepCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner)
+	bicepCli, err := bicep.NewCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner)
 	require.NoError(t, err)
 	azCli := mockazcli.NewAzCliFromMockContext(mockContext)
 	depOpService := mockazcli.NewDeploymentOperationsServiceFromMockContext(mockContext)
@@ -425,7 +425,7 @@ func prepareBicepMocks(
 	mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool {
 		return strings.Contains(args.Cmd, "bicep") && args.Args[0] == "--version"
 	}).Respond(exec.RunResult{
-		Stdout: fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.BicepVersion.String()),
+		Stdout: fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.Version.String()),
 		Stderr: "",
 	})
 
@@ -809,7 +809,7 @@ func TestFindCompletedDeployments(t *testing.T) {
 	mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool {
 		return strings.Contains(args.Cmd, "bicep") && strings.Contains(command, "--version")
 	}).RespondFn(func(args exec.RunArgs) (exec.RunResult, error) {
-		return exec.NewRunResult(0, fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.BicepVersion), ""), nil
+		return exec.NewRunResult(0, fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.Version), ""), nil
 	})
 	// Have `bicep build` return a ARM template that targets a resource group.
 	mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool {
@@ -907,11 +907,11 @@ func TestUserDefinedTypes(t *testing.T) {
 	mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool {
 		return strings.Contains(args.Cmd, "bicep") && args.Args[0] == "--version"
 	}).Respond(exec.RunResult{
-		Stdout: fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.BicepVersion.String()),
+		Stdout: fmt.Sprintf("Bicep CLI version %s (abcdef0123)", bicep.Version.String()),
 		Stderr: "",
 	})
 
-	bicepCli, err := bicep.NewBicepCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner)
+	bicepCli, err := bicep.NewCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner)
 	require.NoError(t, err)
 	env := environment.NewWithValues("test-env", map[string]string{})
 

cli/azd/pkg/tools/bicep/bicep.go

@@ -25,38 +25,33 @@ import (
 	"github.com/blang/semver/v4"
 )
 
-// BicepVersion is the minimum version of bicep that we require (and the one we fetch when we fetch bicep on behalf of a
+// Version is the minimum version of bicep that we require (and the one we fetch when we fetch bicep on behalf of a
 // user).
-var BicepVersion semver.Version = semver.MustParse("0.28.1")
+var Version semver.Version = semver.MustParse("0.28.1")
 
-type BicepCli interface {
-	Build(ctx context.Context, file string) (BuildResult, error)
-	BuildBicepParam(ctx context.Context, file string, env []string) (BuildResult, error)
-}
-
-// NewBicepCli creates a new BicepCli. Azd manages its own copy of the bicep CLI, stored in `$AZD_CONFIG_DIR/bin`. If
+// NewCli creates a new Bicep CLI. Azd manages its own copy of the bicep CLI, stored in `$AZD_CONFIG_DIR/bin`. If
 // bicep is not present at this location, or if it is present but is older than the minimum supported version, it is
 // downloaded.
-func NewBicepCli(
+func NewCli(
 	ctx context.Context,
 	console input.Console,
 	commandRunner exec.CommandRunner,
-) (BicepCli, error) {
-	return newBicepCliWithTransporter(ctx, console, commandRunner, http.DefaultClient)
+) (*Cli, error) {
+	return newCliWithTransporter(ctx, console, commandRunner, http.DefaultClient)
 }
 
-// newBicepCliWithTransporter is like NewBicepCli but allows providing a custom transport to use when downloading the
-// bicep CLI, for testing purposes.
-func newBicepCliWithTransporter(
+// newCliWithTransporter is like NewBicepCli but allows providing a custom transport to use when downloading the
+// Bicep CLI, for testing purposes.
+func newCliWithTransporter(
 	ctx context.Context,
 	console input.Console,
 	commandRunner exec.CommandRunner,
 	transporter policy.Transporter,
-) (BicepCli, error) {
+) (*Cli, error) {
 	if override := os.Getenv("AZD_BICEP_TOOL_PATH"); override != "" {
 		log.Printf("using external bicep tool: %s", override)
 
-		return &bicepCli{
+		return &Cli{
 			path:   override,
 			runner: commandRunner,
 		}, nil
@@ -77,14 +72,14 @@ func newBicepCliWithTransporter(
 
 		if err := runStep(
 			ctx, console, "Downloading Bicep", func() error {
-				return downloadBicep(ctx, transporter, BicepVersion, bicepPath)
+				return downloadBicep(ctx, transporter, Version, bicepPath)
 			},
 		); err != nil {
 			return nil, fmt.Errorf("downloading bicep: %w", err)
 		}
 	}
 
-	cli := &bicepCli{
+	cli := &Cli{
 		path:   bicepPath,
 		runner: commandRunner,
 	}
@@ -96,12 +91,12 @@ func newBicepCliWithTransporter(
 
 	log.Printf("bicep version: %s", ver)
 
-	if ver.LT(BicepVersion) {
-		log.Printf("installed bicep version %s is older than %s; updating.", ver.String(), BicepVersion.String())
+	if ver.LT(Version) {
+		log.Printf("installed bicep version %s is older than %s; updating.", ver.String(), Version.String())
 
 		if err := runStep(
 			ctx, console, "Upgrading Bicep", func() error {
-				return downloadBicep(ctx, transporter, BicepVersion, bicepPath)
+				return downloadBicep(ctx, transporter, Version, bicepPath)
 			},
 		); err != nil {
 			return nil, fmt.Errorf("upgrading bicep: %w", err)
@@ -127,7 +122,7 @@ func runStep(ctx context.Context, console input.Console, title string, action fu
 	return nil
 }
 
-type bicepCli struct {
+type Cli struct {
 	path   string
 	runner exec.CommandRunner
 }
@@ -245,7 +240,7 @@ func preferMuslBicep(stat stater) bool {
 	return false
 }
 
-func (cli *bicepCli) version(ctx context.Context) (semver.Version, error) {
+func (cli *Cli) version(ctx context.Context) (semver.Version, error) {
 	bicepRes, err := cli.runCommand(ctx, nil, "--version")
 	if err != nil {
 		return semver.Version{}, err
@@ -268,7 +263,7 @@ type BuildResult struct {
 	LintErr string
 }
 
-func (cli *bicepCli) Build(ctx context.Context, file string) (BuildResult, error) {
+func (cli *Cli) Build(ctx context.Context, file string) (BuildResult, error) {
 	args := []string{"build", file, "--stdout"}
 	buildRes, err := cli.runCommand(ctx, nil, args...)
 
@@ -285,7 +280,7 @@ func (cli *bicepCli) Build(ctx context.Context, file string) (BuildResult, error
 	}, nil
 }
 
-func (cli *bicepCli) BuildBicepParam(ctx context.Context, file string, env []string) (BuildResult, error) {
+func (cli *Cli) BuildBicepParam(ctx context.Context, file string, env []string) (BuildResult, error) {
 	args := []string{"build-params", file, "--stdout"}
 	buildRes, err := cli.runCommand(ctx, env, args...)
 
@@ -302,7 +297,7 @@ func (cli *bicepCli) BuildBicepParam(ctx context.Context, file string, env []str
 	}, nil
 }
 
-func (cli *bicepCli) runCommand(ctx context.Context, env []string, args ...string) (exec.RunResult, error) {
+func (cli *Cli) runCommand(ctx context.Context, env []string, args ...string) (exec.RunResult, error) {
 	runArgs := exec.NewRunArgs(cli.path, args...)
 	if env != nil {
 		runArgs = runArgs.WithEnv(env)

cli/azd/pkg/tools/bicep/bicep_test.go

@@ -38,11 +38,11 @@ func TestNewBicepCli(t *testing.T) {
 		return strings.Contains(args.Cmd, "bicep") && len(args.Args) == 1 && args.Args[0] == "--version"
 	}).Respond(exec.NewRunResult(
 		0,
-		fmt.Sprintf("Bicep CLI version %s (abcdef0123)", BicepVersion.String()),
+		fmt.Sprintf("Bicep CLI version %s (abcdef0123)", Version.String()),
 		"",
 	))
 
-	cli, err := newBicepCliWithTransporter(
+	cli, err := newCliWithTransporter(
 		*mockContext.Context, mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient,
 	)
 	require.NoError(t, err)
@@ -108,13 +108,13 @@ func TestNewBicepCliWillUpgrade(t *testing.T) {
 		case OLD_FILE_CONTENTS:
 			return exec.NewRunResult(0, "Bicep CLI version 0.0.1 (badbadbad1)", ""), nil
 		case NEW_FILE_CONTENTS:
-			return exec.NewRunResult(0, fmt.Sprintf("Bicep CLI version %s (abcdef0123)", BicepVersion.String()), ""), nil
+			return exec.NewRunResult(0, fmt.Sprintf("Bicep CLI version %s (abcdef0123)", Version.String()), ""), nil
 		}
 
 		return exec.NewRunResult(-1, "", "unexpected bicep file contents"), err
 	})
 
-	cli, err := newBicepCliWithTransporter(
+	cli, err := newCliWithTransporter(
 		*mockContext.Context, mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient,
 	)
 	require.NoError(t, err)

cli/azd/test/functional/aspire_test.go

@@ -207,7 +207,7 @@ func Test_CLI_Aspire_DetectGen(t *testing.T) {
 		_, err = cli.RunCommand(ctx, "infra", "synth")
 		require.NoError(t, err)
 
-		bicepCli, err := bicep.NewBicepCli(ctx, mockinput.NewMockConsole(), exec.NewCommandRunner(nil))
+		bicepCli, err := bicep.NewCli(ctx, mockinput.NewMockConsole(), exec.NewCommandRunner(nil))
 		require.NoError(t, err)
 
 		// Validate bicep builds without errors