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..098444db 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,23 @@ 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 { + v := clients.Config.ManifestSourceFlag + if v == "" { + return nil + } + if !(v == string(config.ManifestSourceLocal) || v == string(config.ManifestSourceRemote)) { + return slackerror.New(slackerror.ErrInvalidFlag). + WithMessage("Invalid value %q for %s flag", v, 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..ea732278 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -55,6 +55,7 @@ type Config struct { ForceFlag bool ForceRemoteFlag bool LogstashHostResolved string + ManifestSourceFlag string NoColor bool RuntimeFlag string RuntimeName string diff --git a/internal/manifest/sync.go b/internal/manifest/sync.go index feb30e15..a7aed42d 100644 --- a/internal/manifest/sync.go +++ b/internal/manifest/sync.go @@ -36,6 +36,15 @@ type SyncResult struct { // both manifests, computes diffs, prompts the user for resolution, writes // the merged result to both the API and the local file, and returns the result. func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, auth types.SlackAuth) (*SyncResult, error) { + if v := clients.Config.ManifestSourceFlag; v != "" && !(v == string(config.ManifestSourceLocal) || v == string(config.ManifestSourceRemote)) { + return nil, slackerror.New(slackerror.ErrInvalidFlag). + WithMessage("Invalid value %q for %s flag", v, style.CommandText("--manifest-source")). + WithRemediation("Valid values are %s or %s", + style.Highlight(string(config.ManifestSourceLocal)), + style.Highlight(string(config.ManifestSourceRemote)), + ) + } + manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx) if err != nil { return nil, err @@ -77,12 +86,12 @@ func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, aut var merged types.AppManifest switch { - case clients.Config.ForceFlag: + case clients.Config.ManifestSourceFlag == string(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 clients.Config.ManifestSourceFlag == string(config.ManifestSourceRemote) || clients.Config.ForceRemoteFlag: merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllRemote) if err != nil { return nil, err @@ -91,8 +100,8 @@ func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, aut 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..f2e1be3c 100644 --- a/internal/manifest/sync_test.go +++ b/internal/manifest/sync_test.go @@ -214,6 +214,83 @@ func Test_Sync(t *testing.T) { }) } + t.Run("manifest-source=local merges all local and pushes to API", 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 = string(config.ManifestSourceLocal) + 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) + f.cacheMock.On("SetManifestHash", mock.Anything, mock.Anything, mock.Anything).Return(nil) + _ = afero.WriteFile(f.fs, "/project/manifest.json", []byte(`{"display_information":{"name":"App"}}`), 0644) + + result, err := Sync(f.ctx, f.clients, testApp, testAuth) + + require.NoError(t, err) + require.NotNil(t, result) + assert.True(t, result.HasDifferences) + assert.Equal(t, "Local", result.Merged.DisplayInformation.Description) + }) + + t.Run("manifest-source=remote merges all remote and pushes to API", 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 = string(config.ManifestSourceRemote) + 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) + f.cacheMock.On("SetManifestHash", mock.Anything, mock.Anything, mock.Anything).Return(nil) + _ = afero.WriteFile(f.fs, "/project/manifest.json", []byte(`{"display_information":{"name":"App"}}`), 0644) + + result, err := Sync(f.ctx, f.clients, testApp, testAuth) + + require.NoError(t, err) + require.NotNil(t, result) + assert.True(t, result.HasDifferences) + assert.Equal(t, "Remote", result.Merged.DisplayInformation.Description) + }) + + 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..460c30b6 100644 --- a/internal/pkg/apps/install.go +++ b/internal/pkg/apps/install.go @@ -701,6 +701,12 @@ func shouldUpdateManifest(ctx context.Context, clients *shared.ClientFactory, ap if err != nil { return false, err } + if clients.Config.ManifestSourceFlag == string(config.ManifestSourceRemote) { + return false, nil + } + if clients.Config.ManifestSourceFlag == string(config.ManifestSourceLocal) { + return true, nil + } if manifestSource.Equals(config.ManifestSourceRemote) { return false, nil }