diff --git a/cmd/manifest/sync.go b/cmd/manifest/sync.go index 3b78a359..6ea0e0ee 100644 --- a/cmd/manifest/sync.go +++ b/cmd/manifest/sync.go @@ -18,6 +18,7 @@ import ( "github.com/opentracing/opentracing-go" "github.com/slackapi/slack-cli/internal/app" "github.com/slackapi/slack-cli/internal/cmdutil" + "github.com/slackapi/slack-cli/internal/config" "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/manifest" "github.com/slackapi/slack-cli/internal/prompts" @@ -37,8 +38,8 @@ func NewSyncCommand(clients *shared.ClientFactory) *cobra.Command { Hidden: true, Example: style.ExampleCommandsf([]style.ExampleCommand{ {Command: "manifest sync", Meaning: "Sync project manifest with app settings"}, - {Command: "manifest sync --force", Meaning: "Push project manifest to app settings without prompting"}, - {Command: "manifest sync --force-remote", Meaning: "Pull app settings to project manifest without prompting"}, + {Command: "manifest sync --manifest-source=local", Meaning: "Push project manifest to app settings without prompting"}, + {Command: "manifest sync --manifest-source=remote", Meaning: "Pull app settings to project manifest without prompting"}, }), Args: cobra.NoArgs, PreRunE: func(cmd *cobra.Command, args []string) error { @@ -49,9 +50,8 @@ func NewSyncCommand(clients *shared.ClientFactory) *cobra.Command { style.CommandText("--experiment manifest-sync"), ) } - if clients.Config.ForceFlag && clients.Config.ForceRemoteFlag { - return slackerror.New(slackerror.ErrMismatchedFlags). - WithMessage("Cannot use both %s and %s flags", style.CommandText("--force"), style.CommandText("--force-remote")) + if err := cmdutil.ValidateManifestSourceFlag(clients); err != nil { + return err } return cmdutil.IsValidProjectDirectory(clients) }, @@ -71,6 +71,6 @@ func NewSyncCommand(clients *shared.ClientFactory) *cobra.Command { return err }, } - cmd.Flags().BoolVar(&clients.Config.ForceRemoteFlag, "force-remote", false, "use all app settings values without prompting") + cmd.Flags().StringVar(&clients.Config.ManifestSourceFlag, "manifest-source", "", "resolve manifest differences using this source ("+string(config.ManifestSourceLocal)+" or "+string(config.ManifestSourceRemote)+")") return cmd } diff --git a/cmd/manifest/sync_test.go b/cmd/manifest/sync_test.go index 2ca0da96..464f6b0c 100644 --- a/cmd/manifest/sync_test.go +++ b/cmd/manifest/sync_test.go @@ -45,15 +45,14 @@ func TestSyncCommand(t *testing.T) { // the gate itself should pass. ExpectedErrorStrings: []string{}, }, - "errors when both --force and --force-remote are set": { - CmdArgs: []string{"--force-remote"}, + "errors when --manifest-source has an invalid value": { + CmdArgs: []string{"--manifest-source=invalid"}, Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { cm.AddDefaultMocks() cf.Config.ExperimentsFlag = []string{string(experiment.ManifestSync)} cf.Config.LoadExperiments(ctx, cf.IO.PrintDebug) - cf.Config.ForceFlag = true }, - ExpectedErrorStrings: []string{"Cannot use both", "--force", "--force-remote"}, + ExpectedErrorStrings: []string{"Invalid value", "invalid", "--manifest-source"}, }, }, func(clients *shared.ClientFactory) *cobra.Command { return NewSyncCommand(clients) diff --git a/cmd/platform/deploy.go b/cmd/platform/deploy.go index 88eed572..d19a9971 100644 --- a/cmd/platform/deploy.go +++ b/cmd/platform/deploy.go @@ -59,6 +59,9 @@ func NewDeployCommand(clients *shared.ClientFactory) *cobra.Command { {Command: "platform deploy --team T0123456", Meaning: "Deploy to a specific team"}, }), PreRunE: func(cmd *cobra.Command, args []string) error { + if err := cmdutil.ValidateManifestSourceFlag(clients); err != nil { + return err + } return cmdutil.IsValidProjectDirectory(clients) }, RunE: func(cmd *cobra.Command, args []string) error { @@ -108,6 +111,7 @@ func NewDeployCommand(clients *shared.ClientFactory) *cobra.Command { } cmd.Flags().BoolVar(&deployFlags.hideTriggers, "hide-triggers", false, "do not list triggers and skip trigger creation prompts") + cmd.Flags().StringVar(&clients.Config.ManifestSourceFlag, "manifest-source", "", "resolve manifest differences using this source (local or remote)") cmd.Flags().StringVar(&deployFlags.orgGrantWorkspaceID, cmdutil.OrgGrantWorkspaceFlag, "", cmdutil.OrgGrantWorkspaceDescription()) return cmd diff --git a/cmd/platform/deploy_test.go b/cmd/platform/deploy_test.go index 00a656e0..8c0e8f4e 100644 --- a/cmd/platform/deploy_test.go +++ b/cmd/platform/deploy_test.go @@ -106,6 +106,24 @@ func TestDeployCommand(t *testing.T) { deployPkgMock.AssertCalled(t, "Deploy", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } +func TestDeployCommand_ManifestSourceFlag_InvalidValue(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + clientsMock := shared.NewClientsMock() + clientsMock.AddDefaultMocks() + clients := shared.NewClientFactory(clientsMock.MockClientFactory(), func(clients *shared.ClientFactory) { + clients.Config.ProjectConfig = config.NewProjectConfigMock() + clients.SDKConfig = hooks.NewSDKConfigMock() + }) + + cmd := NewDeployCommand(clients) + testutil.MockCmdIO(clients.IO, cmd) + cmd.SetArgs([]string{"--manifest-source", "invalid"}) + + err := cmd.ExecuteContext(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid") +} + func TestDeployCommand_HasValidDeploymentMethod(t *testing.T) { tests := map[string]struct { app types.App diff --git a/cmd/platform/run.go b/cmd/platform/run.go index 35d258fa..dcc683a4 100644 --- a/cmd/platform/run.go +++ b/cmd/platform/run.go @@ -58,7 +58,9 @@ func NewRunCommand(clients *shared.ClientFactory) *cobra.Command { {Command: "platform run --cleanup", Meaning: "Run a local development server with cleanup"}, }), PreRunE: func(cmd *cobra.Command, args []string) error { - // Verify command is run in a project directory + if err := cmdutil.ValidateManifestSourceFlag(clients); err != nil { + return err + } return cmdutil.IsValidProjectDirectory(clients) }, RunE: func(cmd *cobra.Command, args []string) error { @@ -70,6 +72,7 @@ func NewRunCommand(clients *shared.ClientFactory) *cobra.Command { cmd.Flags().StringVar(&runFlags.activityLevel, "activity-level", platform.ActivityMinLevelDefault, "activity level to display") cmd.Flags().BoolVar(&runFlags.noActivity, "no-activity", false, "hide Slack Platform log activity") cmd.Flags().BoolVar(&runFlags.cleanup, "cleanup", false, "uninstall the local app after exiting") + cmd.Flags().StringVar(&clients.Config.ManifestSourceFlag, "manifest-source", "", "resolve manifest differences using this source (local or remote)") cmd.Flags().StringVar(&runFlags.orgGrantWorkspaceID, cmdutil.OrgGrantWorkspaceFlag, "", cmdutil.OrgGrantWorkspaceDescription()) cmd.Flags().BoolVar(&runFlags.hideTriggers, "hide-triggers", false, "do not list triggers and skip trigger creation prompts") diff --git a/cmd/platform/run_test.go b/cmd/platform/run_test.go index 517b5c92..21777600 100644 --- a/cmd/platform/run_test.go +++ b/cmd/platform/run_test.go @@ -32,6 +32,7 @@ import ( "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) // Setup a mock for the package @@ -277,6 +278,24 @@ func TestRunCommand_Flags(t *testing.T) { } } +func TestRunCommand_ManifestSourceFlag_InvalidValue(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + clientsMock := shared.NewClientsMock() + clientsMock.IO.On("IsTTY").Return(true) + clientsMock.IO.AddDefaultMocks() + clients := shared.NewClientFactory(clientsMock.MockClientFactory(), func(clients *shared.ClientFactory) { + clients.SDKConfig = hooks.NewSDKConfigMock() + }) + + cmd := NewRunCommand(clients) + testutil.MockCmdIO(clients.IO, cmd) + cmd.SetArgs([]string{"--manifest-source", "invalid"}) + + err := cmd.ExecuteContext(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid") +} + func TestRunCommand_Help(t *testing.T) { ctx := slackcontext.MockContext(t.Context()) clientsMock := shared.NewClientsMock() diff --git a/internal/cmdutil/flags.go b/internal/cmdutil/flags.go index cdbea12a..c1fe9f86 100644 --- a/internal/cmdutil/flags.go +++ b/internal/cmdutil/flags.go @@ -17,6 +17,9 @@ package cmdutil import ( "fmt" + "github.com/slackapi/slack-cli/internal/config" + "github.com/slackapi/slack-cli/internal/shared" + "github.com/slackapi/slack-cli/internal/slackerror" "github.com/slackapi/slack-cli/internal/style" "github.com/spf13/cobra" ) @@ -35,6 +38,20 @@ var OrgGrantWorkspaceDescription = func() string { style.Secondary("(or 'all' for all workspaces in the org)")) } +// ValidateManifestSourceFlag checks that --manifest-source has a valid value if set +func ValidateManifestSourceFlag(clients *shared.ClientFactory) error { + ms := config.ManifestSource(clients.Config.ManifestSourceFlag) + if ms.Exists() && !ms.IsValid() { + return slackerror.New(slackerror.ErrInvalidFlag). + WithMessage("Invalid value %q for %s flag", clients.Config.ManifestSourceFlag, style.CommandText("--manifest-source")). + WithRemediation("Valid values are %s or %s", + style.Highlight(string(config.ManifestSourceLocal)), + style.Highlight(string(config.ManifestSourceRemote)), + ) + } + return nil +} + // IsFlagChanged checks if a certain flag has been set in the command func IsFlagChanged(cmd *cobra.Command, flag string) bool { IsFlagSet := cmd.Flags().Lookup(flag) diff --git a/internal/cmdutil/flags_test.go b/internal/cmdutil/flags_test.go index 2189bc95..bc4727ba 100644 --- a/internal/cmdutil/flags_test.go +++ b/internal/cmdutil/flags_test.go @@ -17,10 +17,55 @@ package cmdutil import ( "testing" + "github.com/slackapi/slack-cli/internal/config" + "github.com/slackapi/slack-cli/internal/shared" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func Test_ValidateManifestSourceFlag(t *testing.T) { + tests := map[string]struct { + value string + expectErr bool + }{ + "flag not provided is valid": { + value: "", + expectErr: false, + }, + "local is valid": { + value: "local", + expectErr: false, + }, + "remote is valid": { + value: "remote", + expectErr: false, + }, + "invalid value returns error": { + value: "invalid", + expectErr: true, + }, + "project is not valid": { + value: "project", + expectErr: true, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + clients := &shared.ClientFactory{ + Config: &config.Config{ManifestSourceFlag: tc.value}, + } + err := ValidateManifestSourceFlag(clients) + if tc.expectErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.value) + } else { + require.NoError(t, err) + } + }) + } +} + func Test_IsFlagChanged(t *testing.T) { tests := map[string]struct { flag string diff --git a/internal/config/config.go b/internal/config/config.go index 979a0afe..99fbe23e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -53,8 +53,8 @@ type Config struct { DeprecatedWorkspaceFlag string DisableTelemetryFlag bool ForceFlag bool - ForceRemoteFlag bool LogstashHostResolved string + ManifestSourceFlag string NoColor bool RuntimeFlag string RuntimeName string diff --git a/internal/config/manifest.go b/internal/config/manifest.go index 5a1669dd..cd7c1c80 100644 --- a/internal/config/manifest.go +++ b/internal/config/manifest.go @@ -37,6 +37,11 @@ func (ms ManifestSource) String() string { return string(ms) } +// IsValid returns true if the manifest source is a known valid value +func (ms ManifestSource) IsValid() bool { + return ms.Equals(ManifestSourceLocal) || ms.Equals(ManifestSourceRemote) +} + // Human returns the string value as a human-friendly name func (ms ManifestSource) Human() string { switch ms { diff --git a/internal/manifest/sync.go b/internal/manifest/sync.go index feb30e15..ec3eb31d 100644 --- a/internal/manifest/sync.go +++ b/internal/manifest/sync.go @@ -76,23 +76,31 @@ func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, aut DisplayDiffs(ctx, clients.IO, diffs) var merged types.AppManifest + flagSource := config.ManifestSource(clients.Config.ManifestSourceFlag) switch { - case clients.Config.ForceFlag: + case flagSource.Equals(config.ManifestSourceLocal) || clients.Config.ForceFlag: merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllLocal) if err != nil { return nil, err } - case clients.Config.ForceRemoteFlag: + case flagSource.Equals(config.ManifestSourceRemote): merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllRemote) if err != nil { return nil, err } + case flagSource.Exists() && !flagSource.IsValid(): + return nil, slackerror.New(slackerror.ErrInvalidFlag). + WithMessage("Invalid value %q for %s flag", clients.Config.ManifestSourceFlag, style.CommandText("--manifest-source")). + WithRemediation("Valid values are %s or %s", + style.Highlight(string(config.ManifestSourceLocal)), + style.Highlight(string(config.ManifestSourceRemote)), + ) case !clients.IO.IsTTY(): return nil, slackerror.New(slackerror.ErrAppManifestUpdate). WithRemediation("Run %s interactively to resolve manifest differences, or pass %s to push the project manifest to app settings or %s to pull app settings to project", style.Commandf("manifest sync", false), - style.CommandText("--force"), - style.CommandText("--force-remote"), + style.CommandText("--manifest-source=local"), + style.CommandText("--manifest-source=remote"), ) default: merged, err = resolveInteractively(ctx, clients, localManifest.AppManifest, remoteManifest.AppManifest, diffs) diff --git a/internal/manifest/sync_test.go b/internal/manifest/sync_test.go index 7d1b5265..e6674cd2 100644 --- a/internal/manifest/sync_test.go +++ b/internal/manifest/sync_test.go @@ -174,17 +174,21 @@ func Test_Sync(t *testing.T) { }) mergeStrategyTests := map[string]struct { - forceFlag bool - forceRemoteFlag bool - expectedDesc string + forceFlag bool + manifestSourceFlag string + expectedDesc string }{ "force flag merges all local and pushes to API": { forceFlag: true, expectedDesc: "Local", }, - "force-remote flag merges all remote and pushes to API": { - forceRemoteFlag: true, - expectedDesc: "Remote", + "manifest-source=local merges all local and pushes to API": { + manifestSourceFlag: string(config.ManifestSourceLocal), + expectedDesc: "Local", + }, + "manifest-source=remote merges all remote and pushes to API": { + manifestSourceFlag: string(config.ManifestSourceRemote), + expectedDesc: "Remote", }, } for name, tc := range mergeStrategyTests { @@ -196,7 +200,7 @@ func Test_Sync(t *testing.T) { f.manifestMock.On("GetManifestRemote", mock.Anything, mock.Anything, mock.Anything). Return(remoteManifest, nil) f.clients.Config.ForceFlag = tc.forceFlag - f.clients.Config.ForceRemoteFlag = tc.forceRemoteFlag + f.clients.Config.ManifestSourceFlag = tc.manifestSourceFlag f.clientsMock.API.On("UpdateApp", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(api.UpdateAppResult{}, nil) f.cacheMock.On("NewManifestHash", mock.Anything, mock.Anything).Return(cache.Hash("newhash"), nil) @@ -214,6 +218,39 @@ func Test_Sync(t *testing.T) { }) } + t.Run("invalid manifest-source flag returns error", func(t *testing.T) { + f := newSyncTestFixture(t) + f.projectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil) + f.manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything). + Return(localManifest, nil) + f.manifestMock.On("GetManifestRemote", mock.Anything, mock.Anything, mock.Anything). + Return(remoteManifest, nil) + f.clients.Config.ManifestSourceFlag = "invalid" + + result, err := Sync(f.ctx, f.clients, testApp, testAuth) + + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "invalid") + assert.Contains(t, err.Error(), "--manifest-source") + }) + + t.Run("non-TTY error mentions --manifest-source in remediation", func(t *testing.T) { + f := newSyncTestFixture(t) + f.projectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil) + f.manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything). + Return(localManifest, nil) + f.manifestMock.On("GetManifestRemote", mock.Anything, mock.Anything, mock.Anything). + Return(remoteManifest, nil) + + _, err := Sync(f.ctx, f.clients, testApp, testAuth) + + require.Error(t, err) + slackErr := slackerror.ToSlackError(err) + assert.Contains(t, slackErr.Remediation, "--manifest-source=local") + assert.Contains(t, slackErr.Remediation, "--manifest-source=remote") + }) + t.Run("API UpdateApp failure is propagated", func(t *testing.T) { f := newSyncTestFixture(t) f.projectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil) diff --git a/internal/pkg/apps/install.go b/internal/pkg/apps/install.go index 88f7237d..17c57572 100644 --- a/internal/pkg/apps/install.go +++ b/internal/pkg/apps/install.go @@ -701,6 +701,13 @@ func shouldUpdateManifest(ctx context.Context, clients *shared.ClientFactory, ap if err != nil { return false, err } + flagSource := config.ManifestSource(clients.Config.ManifestSourceFlag) + if flagSource.Equals(config.ManifestSourceRemote) { + return false, nil + } + if flagSource.Equals(config.ManifestSourceLocal) { + return true, nil + } if manifestSource.Equals(config.ManifestSourceRemote) { return false, nil }