Clean application config of Nomad related methods (#3257)

* fly config validate * Remove SetNomadPlatform and SetPlatformVersion methods * Remove SetNomadPlatform and SetDetachedPlatform methods * The less GQL calsl the better * Remove Nomad and Detached from config validation * the less GQL calls the better * remove nomad and detached platforms from appconfig ProcessGroups * Remove appconfig.Config.RawDefinition * remove unused func * No more appconfig.*Platform constants * Remove nomad from comments
superfly · Feb 9, 2024 · 711ca1f · 711ca1f
1 parent 6ee490a
commit 711ca1f
Show file tree

Hide file tree

Showing 30 changed files with 73 additions and 987 deletions.
diff --git a/internal/appconfig/config.go b/internal/appconfig/config.go
@@ -17,26 +17,15 @@ import (
 const (
 	// DefaultConfigFileName denotes the default application configuration file name.
 	DefaultConfigFileName = "fly.toml"
-	// Config is versioned, initially, to separate nomad from machine apps without having to consult
-	// the API
-	MachinesPlatform = "machines"
-	NomadPlatform    = "nomad"
-	DetachedPlatform = "detached"
 )
 
 func NewConfig() *Config {
 	return &Config{
-		RawDefinition:    map[string]any{},
 		defaultGroupName: api.MachineProcessGroupApp,
 		configFilePath:   "--config path unset--",
 	}
 }
 
-type Metrics struct {
-	*api.MachineMetrics
-	Processes []string `json:"processes,omitempty" toml:"processes,omitempty"`
-}
-
 // Config wraps the properties of app configuration.
 // NOTE: If you any new setting here, please also add a value for it at testdata/rull-reference.toml
 type Config struct {
@@ -68,27 +57,24 @@ type Config struct {
 	Statics []Static   `toml:"statics,omitempty" json:"statics,omitempty"`
 	Metrics []*Metrics `toml:"metrics,omitempty" json:"metrics,omitempty"`
 
-	// RawDefinition contains fly.toml parsed as-is
-	// If you add any config field that is v2 specific, be sure to remove it in SanitizeDefinition()
-	RawDefinition map[string]any `toml:"-" json:"-"`
-
 	// MergedFiles is a list of files that have been merged from the app config and flags.
 	MergedFiles []*api.File `toml:"-" json:"-"`
 
 	// Path to application configuration file, usually fly.toml.
 	configFilePath string
 
-	// Indicates the intended platform to use: machines or nomad
-	platformVersion string
-
 	// Set when it fails to unmarshal fly.toml into Config
-	// Don't hard fail because RawDefinition still holds the app configuration for Nomad apps
 	v2UnmarshalError error
 
 	// The default group name to refer to (used with flatten configs)
 	defaultGroupName string
 }
 
+type Metrics struct {
+	*api.MachineMetrics
+	Processes []string `json:"processes,omitempty" toml:"processes,omitempty"`
+}
+
 type Deploy struct {
 	ReleaseCommand        string        `toml:"release_command,omitempty" json:"release_command,omitempty"`
 	ReleaseCommandTimeout *api.Duration `toml:"release_command_timeout,omitempty" json:"release_command_timeout,omitempty"`
@@ -313,10 +299,6 @@ func (cfg *Config) URL() *url.URL {
 	}
 }
 
-func (cfg *Config) PlatformVersion() string {
-	return cfg.platformVersion
-}
-
 // MergeFiles merges the provided files with the files in the config wherein the provided files
 // take precedence.
 func (cfg *Config) MergeFiles(files []*api.File) error {

diff --git a/internal/appconfig/config_test.go b/internal/appconfig/config_test.go
@@ -133,18 +133,6 @@ func TestConfigPortGetter(t *testing.T) {
 func TestCloneAppconfig(t *testing.T) {
 	config := &Config{
 		AppName: "testcfg",
-		RawDefinition: map[string]any{
-			"mounts": []Mount{
-				{
-					Source:      "src-raw",
-					Destination: "dst-raw",
-				},
-				{
-					Source:      "src2",
-					Destination: "dst2",
-				},
-			},
-		},
 		Mounts: []Mount{{
 			Source:      "src",
 			Destination: "dst",

diff --git a/internal/appconfig/definition.go b/internal/appconfig/definition.go
@@ -2,7 +2,6 @@ package appconfig
 
 import (
 	"github.com/pelletier/go-toml/v2"
-	"github.com/samber/lo"
 	"github.com/superfly/flyctl/api"
 )
 
@@ -27,16 +26,3 @@ func FromDefinition(definition *api.Definition) (*Config, error) {
 	}
 	return unmarshalTOML(buf)
 }
-
-// SanitizedDefinition returns a definition cleaned from any extra fields
-// not valid for Web API GQL endpoints.
-func (c *Config) SanitizedDefinition() map[string]any {
-	// Beware this is a shallow Copy
-	definition := lo.Assign(c.RawDefinition)
-	delete(definition, "app")
-	delete(definition, "build")
-	delete(definition, "primary_region")
-	delete(definition, "http_service")
-	delete(definition, "console_command")
-	return definition
-}
diff --git a/internal/appconfig/definition_test.go b/internal/appconfig/definition_test.go
@@ -109,48 +109,6 @@ func TestFromDefinition(t *testing.T) {
 		},
 		configFilePath:   "--config path unset--",
 		defaultGroupName: "app",
-		RawDefinition: map[string]any{
-			"env": map[string]any{},
-			"experimental": map[string]any{
-				"auto_rollback": true,
-			},
-			"kill_signal":  "SIGINT",
-			"kill_timeout": float64(5),
-			"processes":    []any{},
-			"services": []any{
-				map[string]any{
-					"concurrency": map[string]any{
-						"hard_limit": float64(25),
-						"soft_limit": float64(20),
-						"type":       "connections",
-					},
-					"http_checks":   []any{},
-					"internal_port": float64(8080),
-					"ports": []any{
-						map[string]any{
-							"force_https": true,
-							"handlers":    []any{"http"},
-							"port":        float64(80),
-						},
-						map[string]any{
-							"handlers": []any{"tls", "http"},
-							"port":     float64(443),
-						},
-					},
-					"processes":     []any{"app"},
-					"protocol":      "tcp",
-					"script_checks": []any{},
-					"tcp_checks": []any{
-						map[string]any{
-							"grace_period":  "1s",
-							"interval":      "15s",
-							"restart_limit": float64(0),
-							"timeout":       "2s",
-						},
-					},
-				},
-			},
-		},
 	}, cfg)
 }
 

diff --git a/internal/appconfig/from_machine_set.go b/internal/appconfig/from_machine_set.go
@@ -16,7 +16,7 @@ import (
 	"github.com/superfly/flyctl/terminal"
 )
 
-func FromAppAndMachineSet(ctx context.Context, appCompact *api.AppCompact, machines machine.MachineSet) (*Config, string, error) {
+func FromAppAndMachineSet(ctx context.Context, appName string, machines machine.MachineSet) (*Config, string, error) {
 	var (
 		warnings                  []string
 		io                        = iostreams.FromContext(ctx)
@@ -28,7 +28,7 @@ func FromAppAndMachineSet(ctx context.Context, appCompact *api.AppCompact, machi
 		warnings = append(warnings, warningMsg)
 	}
 	for _, m := range machines.GetMachines() {
-		appConfig, machineWarning := fromAppAndOneMachine(ctx, appCompact, m, processGroups)
+		appConfig, machineWarning := fromAppAndOneMachine(ctx, appName, m, processGroups)
 		warnings = append(warnings, machineWarning)
 		tomlString, err := appConfig.marshalTOML()
 		if err != nil {
@@ -87,7 +87,7 @@ func prettyDiff(original, new string, colorize *iostreams.ColorScheme) string {
 	return ""
 }
 
-func fromAppAndOneMachine(ctx context.Context, appCompact *api.AppCompact, m machine.LeasableMachine, processGroups *processGroupInfo) (*Config, string) {
+func fromAppAndOneMachine(ctx context.Context, appName string, m machine.LeasableMachine, processGroups *processGroupInfo) (*Config, string) {
 	var (
 		warningMsg     string
 		primaryRegion  string
@@ -130,7 +130,7 @@ fly.toml only supports one mount per machine at this time. These mounts will be
 		}
 	}
 	cfg := NewConfig()
-	cfg.AppName = appCompact.Name
+	cfg.AppName = appName
 	cfg.PrimaryRegion = primaryRegion
 	cfg.Env = m.Machine().Config.Env
 	cfg.Metrics = []*Metrics{

diff --git a/internal/appconfig/platformversion.go b/internal/appconfig/platformversion.go
@@ -1,54 +1,9 @@
 package appconfig
 
-import "fmt"
-
-func (c *Config) EnsureV2Config() error {
-	return c.v2UnmarshalError
-}
-
 // SetMachinesPlatform informs the TOML marshaller that this config is for the machines platform
 func (c *Config) SetMachinesPlatform() error {
 	if c.v2UnmarshalError != nil {
 		return c.v2UnmarshalError
 	}
-	c.platformVersion = MachinesPlatform
-	return nil
-}
-
-// SetMachinesPlatform informs the TOML marshaller that this config is for the machines platform
-func (c *Config) SetDetachedPlatform() error {
-	if c.v2UnmarshalError != nil {
-		return c.v2UnmarshalError
-	}
-	c.platformVersion = DetachedPlatform
 	return nil
 }
-
-// SetNomadPlatform informs the TOML marshaller that this config is for the nomad platform
-func (c *Config) SetNomadPlatform() error {
-	if len(c.RawDefinition) == 0 {
-		return fmt.Errorf("Can't set platformVersion to Nomad on an empty RawDefinition")
-	}
-	c.platformVersion = NomadPlatform
-	return nil
-}
-
-func (c *Config) SetPlatformVersion(platform string) error {
-	switch platform {
-	case MachinesPlatform:
-		return c.SetMachinesPlatform()
-	case NomadPlatform:
-		return c.SetNomadPlatform()
-	case DetachedPlatform:
-		return c.SetDetachedPlatform()
-	case "":
-		return fmt.Errorf("Empty value as platform version")
-	default:
-		return fmt.Errorf("Unknown platform version: '%s'", platform)
-	}
-}
-
-// ForMachines is true when the config is intended for the machines platform
-func (c *Config) ForMachines() bool {
-	return c.platformVersion == MachinesPlatform
-}
diff --git a/internal/appconfig/platformversion_test.go b/internal/appconfig/platformversion_test.go
@@ -10,25 +10,7 @@ import (
 func TestSetMachinesPlatform(t *testing.T) {
 	cfg := NewConfig()
 	assert.NoError(t, cfg.SetMachinesPlatform())
-	assert.NoError(t, cfg.SetPlatformVersion(MachinesPlatform))
 
 	cfg.v2UnmarshalError = fmt.Errorf("Failed to parse fly.toml")
 	assert.Error(t, cfg.SetMachinesPlatform())
-	assert.Error(t, cfg.SetPlatformVersion(MachinesPlatform))
-}
-
-func TestSetNomadPlatform(t *testing.T) {
-	cfg := NewConfig()
-	assert.Error(t, cfg.SetNomadPlatform())
-	assert.Error(t, cfg.SetPlatformVersion(NomadPlatform))
-
-	cfg.RawDefinition["app"] = "foo"
-	assert.NoError(t, cfg.SetNomadPlatform())
-	assert.NoError(t, cfg.SetPlatformVersion(NomadPlatform))
-}
-
-func TestSetPlatformVersion(t *testing.T) {
-	cfg := NewConfig()
-	assert.Error(t, cfg.SetPlatformVersion(""))
-	assert.Error(t, cfg.SetPlatformVersion("thenewkidsontheblock"))
 }
diff --git a/internal/appconfig/processgroups.go b/internal/appconfig/processgroups.go
@@ -13,32 +13,11 @@ import (
 
 // ProcessNames lists each key of c.Processes, sorted lexicographically
 // If c.Processes == nil, returns ["app"]
-func (c *Config) ProcessNames() (names []string) {
-	switch {
-	case c == nil:
+func (c *Config) ProcessNames() []string {
+	if c == nil {
 		return []string{api.MachineProcessGroupApp}
-	case c.platformVersion == MachinesPlatform:
-		if len(c.Processes) != 0 {
-			names = lo.Keys(c.Processes)
-		}
-	case c.platformVersion == "":
-		fallthrough
-	case c.platformVersion == DetachedPlatform:
-		fallthrough
-	case c.platformVersion == NomadPlatform:
-		switch cast := c.RawDefinition["processes"].(type) {
-		case map[string]any:
-			if len(cast) != 0 {
-				names = lo.Keys(cast)
-			}
-		case map[string]string:
-			if len(cast) != 0 {
-				names = lo.Keys(cast)
-			}
-		}
 	}
-
-	switch {
+	switch names := lo.Keys(c.Processes); {
 	case len(names) == 1:
 		return names
 	case len(names) > 1:
@@ -109,7 +88,6 @@ func (c *Config) Flatten(groupName string) (*Config, error) {
 	}
 
 	dst := helpers.Clone(c)
-	dst.platformVersion = c.platformVersion
 	dst.configFilePath = "--flatten--"
 	dst.defaultGroupName = groupName
 

diff --git a/internal/appconfig/processgroups_test.go b/internal/appconfig/processgroups_test.go
@@ -67,34 +67,6 @@ func TestProcessNames(t *testing.T) {
 				cfg, err = LoadConfig(tc.filepath)
 				require.NoError(t, err)
 			}
-			// Test unknown platform version
-			assert.Equal(t, tc.defaultProcessName, cfg.DefaultProcessName())
-			assert.Equal(t, tc.processNames, cfg.ProcessNames())
-			assert.Equal(t, tc.format, cfg.FormatProcessNames())
-
-			// XXX: Break here because SetPlatform calls crash on nil Config
-			if cfg == nil {
-				return
-			}
-
-			// Test for machines
-			require.NoError(t, cfg.SetMachinesPlatform())
-			assert.Equal(t, tc.defaultProcessName, cfg.DefaultProcessName())
-			assert.Equal(t, tc.processNames, cfg.ProcessNames())
-			assert.Equal(t, tc.format, cfg.FormatProcessNames())
-
-			if cfg.RawDefinition == nil {
-				return
-			}
-
-			// Test for detached
-			require.NoError(t, cfg.SetDetachedPlatform())
-			assert.Equal(t, tc.defaultProcessName, cfg.DefaultProcessName())
-			assert.Equal(t, tc.processNames, cfg.ProcessNames())
-			assert.Equal(t, tc.format, cfg.FormatProcessNames())
-
-			// Test for nomad
-			require.NoError(t, cfg.SetNomadPlatform())
 			assert.Equal(t, tc.defaultProcessName, cfg.DefaultProcessName())
 			assert.Equal(t, tc.processNames, cfg.ProcessNames())
 			assert.Equal(t, tc.format, cfg.FormatProcessNames())