Move strategy validation to appconfig.Validate() and show validation …

…errors on deploy

internal/appconfig/validation.go

@@ -13,7 +13,10 @@ import (
 	"golang.org/x/exp/slices"
 )
 
-var ValidationError = errors.New("invalid app configuration")
+var (
+	ValidationError          = errors.New("invalid app configuration")
+	MachinesDeployStrategies = []string{"canary", "rolling", "immediate"}
+)
 
 func (cfg *Config) Validate(ctx context.Context) (err error, extra_info string) {
 	appName := NameFromContext(ctx)
@@ -131,12 +134,30 @@ func (cfg *Config) validateBuildStrategies() (extraInfo string, err error) {
 }
 
 func (cfg *Config) validateDeploySection() (extraInfo string, err error) {
-	if cfg.Deploy != nil {
-		if _, vErr := shlex.Split(cfg.Deploy.ReleaseCommand); vErr != nil {
-			extraInfo += fmt.Sprintf("Can't shell split release command: '%s'\n", cfg.Deploy.ReleaseCommand)
+	if cfg.Deploy == nil {
+		return
+	}
+
+	if _, vErr := shlex.Split(cfg.Deploy.ReleaseCommand); vErr != nil {
+		extraInfo += fmt.Sprintf("Can't shell split release command: '%s'\n", cfg.Deploy.ReleaseCommand)
+		err = ValidationError
+	}
+
+	if s := cfg.Deploy.Strategy; s != "" {
+		if !slices.Contains(MachinesDeployStrategies, s) {
+			extraInfo += fmt.Sprintf(
+				"unsupported deployment strategy '%s'; Apps v2 supports the following strategies: %s", s,
+				strings.Join(MachinesDeployStrategies, ", "),
+			)
+			err = ValidationError
+		}
+
+		if s == "canary" && len(cfg.Mounts) > 0 {
+			extraInfo += "error canary deployment strategy is not supported when using mounted volumes"
 			err = ValidationError
 		}
 	}
+
 	return
 }
 

internal/command/deploy/deploy.go

@@ -176,8 +176,7 @@ func DeployWithConfig(ctx context.Context, appConfig *appconfig.Config, args Dep
 		if err := appConfig.EnsureV2Config(); err != nil {
 			return fmt.Errorf("Can't deploy an invalid v2 app config: %s", err)
 		}
-		err := deployToMachines(ctx, appConfig, appCompact, img)
-		if err != nil {
+		if err := deployToMachines(ctx, appConfig, appCompact, img); err != nil {
 			return err
 		}
 	default:

internal/command/deploy/machines.go

@@ -2,7 +2,6 @@ package deploy
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -20,7 +19,6 @@ import (
 	"github.com/superfly/flyctl/internal/machine"
 	"github.com/superfly/flyctl/iostreams"
 	"github.com/superfly/flyctl/terminal"
-	"golang.org/x/exp/slices"
 )
 
 const (
@@ -82,11 +80,16 @@ func NewMachineDeployment(ctx context.Context, args MachineDeploymentArgs) (Mach
 	if args.RestartOnly && args.DeploymentImage != "" {
 		return nil, fmt.Errorf("BUG: restartOnly machines deployment created and specified an image")
 	}
-	appConfig, err := determineAppConfigForMachines(ctx, args.EnvFromFlags, args.PrimaryRegionFlag)
+	appConfig, err := determineAppConfigForMachines(ctx, args.EnvFromFlags, args.PrimaryRegionFlag, args.Strategy)
 	if err != nil {
 		return nil, err
 	}
-	err, _ = appConfig.Validate(ctx)
+	// TODO: Blend extraInfo into ValidationError and remove this hack
+	if err, extraInfo := appConfig.Validate(ctx); err != nil {
+		fmt.Fprintf(iostreams.FromContext(ctx).ErrOut, extraInfo)
+		return nil, err
+	}
+
 	if err != nil {
 		return nil, err
 	}
@@ -136,10 +139,7 @@ func NewMachineDeployment(ctx context.Context, args MachineDeploymentArgs) (Mach
 		listenAddressChecked:  make(map[string]struct{}),
 		strategy:              "rolling", // default strategy if nothing else is specified
 	}
-	if err := md.setVolumes(ctx); err != nil {
-		return nil, err
-	}
-	if err := md.setStrategy(args.Strategy); err != nil {
+	if err := md.setStrategy(); err != nil {
 		return nil, err
 	}
 	if err := md.setMachineGuest(args.VMSize); err != nil {
@@ -148,6 +148,9 @@ func NewMachineDeployment(ctx context.Context, args MachineDeploymentArgs) (Mach
 	if err := md.setMachinesForDeployment(ctx); err != nil {
 		return nil, err
 	}
+	if err := md.setVolumes(ctx); err != nil {
+		return nil, err
+	}
 	if err := md.setImg(ctx); err != nil {
 		return nil, err
 	}
@@ -389,28 +392,14 @@ func (md *machineDeployment) setMachineGuest(vmSize string) error {
 	return md.machineGuest.SetSize(vmSize)
 }
 
-func (md *machineDeployment) setStrategy(passedInStrategy string) error {
-	if passedInStrategy != "" {
-		md.strategy = passedInStrategy
-	} else if md.appConfig.Deploy != nil && md.appConfig.Deploy.Strategy != "" {
+func (md *machineDeployment) setStrategy() error {
+	md.strategy = "rolling"
+	if md.appConfig.Deploy != nil && md.appConfig.Deploy.Strategy != "" {
 		md.strategy = md.appConfig.Deploy.Strategy
 	}
-
-	if !MachineSupportedStrategy(md.strategy) {
-		return fmt.Errorf("error unsupported deployment strategy '%s'; fly deploy for machines supports rolling and immediate strategies", md.strategy)
-	}
-
-	if len(md.volumes) > 0 && md.strategy == "canary" {
-		return errors.New("error canary deployment strategy is not supported when using volumes")
-	}
-
 	return nil
 }
 
-func MachineSupportedStrategy(strategy string) bool {
-	return slices.Contains([]string{"canary", "rolling", "immmediate", ""}, strategy)
-}
-
 func (md *machineDeployment) createReleaseInBackend(ctx context.Context) error {
 	_ = `# @genqlient
 	mutation MachinesCreateRelease($input:CreateReleaseInput!) {
@@ -467,7 +456,7 @@ func (md *machineDeployment) logClearLinesAbove(count int) {
 	}
 }
 
-func determineAppConfigForMachines(ctx context.Context, envFromFlags []string, primaryRegion string) (*appconfig.Config, error) {
+func determineAppConfigForMachines(ctx context.Context, envFromFlags []string, primaryRegion, strategy string) (*appconfig.Config, error) {
 	appConfig := appconfig.ConfigFromContext(ctx)
 	if appConfig == nil {
 		return nil, fmt.Errorf("BUG: application configuration must come in the context, be sure to pass it before calling NewMachineDeployment")
@@ -482,6 +471,13 @@ func determineAppConfigForMachines(ctx context.Context, envFromFlags []string, p
 		appConfig.SetEnvVariables(parsedEnv)
 	}
 
+	if strategy != "" {
+		if appConfig.Deploy == nil {
+			appConfig.Deploy = &appconfig.Deploy{}
+		}
+		appConfig.Deploy.Strategy = strategy
+	}
+
 	// deleting this block will result in machines not being deployed in the user selected region
 	if primaryRegion != "" {
 		appConfig.PrimaryRegion = primaryRegion

internal/command/launch/launch.go

@@ -22,6 +22,7 @@ import (
 	"github.com/superfly/flyctl/iostreams"
 	"github.com/superfly/flyctl/scanner"
 	"github.com/superfly/graphql"
+	"golang.org/x/exp/slices"
 )
 
 func New() (cmd *cobra.Command) {
@@ -180,7 +181,11 @@ func run(ctx context.Context) (err error) {
 		return err
 	}
 
-	using_appsv1_only_feature := !deploy.MachineSupportedStrategy(flag.GetString(ctx, "strategy"))
+	using_appsv1_only_feature := false
+	if s := flag.GetString(ctx, "strategy"); s != "" && !slices.Contains(appconfig.MachinesDeployStrategies, s) {
+		using_appsv1_only_feature = true
+	}
+
 	if !shouldUseMachines && !using_appsv1_only_feature {
 		fmt.Fprintf(io.ErrOut, "%s Apps v1 Platform is deprecated. We recommend using the --force-machines flag, or setting\nyour organization's default for new apps to Apps v2 with 'fly orgs apps-v2 default-on <org-name>'\n", aurora.Yellow("WARN"))
 	}