Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions cmd/manifest/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 {
Expand All @@ -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)
},
Expand All @@ -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
}
7 changes: 3 additions & 4 deletions cmd/manifest/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions cmd/platform/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions cmd/platform/deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion cmd/platform/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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")

Expand Down
19 changes: 19 additions & 0 deletions cmd/platform/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
17 changes: 17 additions & 0 deletions internal/cmdutil/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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
}
Comment on lines +41 to +53

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👁️‍🗨️ note: Adjacent comment suggests adding "IsValid" to the manifest source configurations that I think might be useful instead of flag specific checks here?

👾 note: I'm not so confident with PreRunE flag checks but am thinking that might return both truth values with an optional error:

func (ms ManifestSource) IsValid() (bool, error)


// 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)
Expand Down
45 changes: 45 additions & 0 deletions internal/cmdutil/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ type Config struct {
DeprecatedWorkspaceFlag string
DisableTelemetryFlag bool
ForceFlag bool
ForceRemoteFlag bool
LogstashHostResolved string
ManifestSourceFlag string
Comment on lines -56 to +57

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌟 praise: Super appreciate keeping shared flags here!

NoColor bool
RuntimeFlag string
RuntimeName string
Expand Down
5 changes: 5 additions & 0 deletions internal/config/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
16 changes: 12 additions & 4 deletions internal/manifest/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
51 changes: 44 additions & 7 deletions internal/manifest/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand All @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions internal/pkg/apps/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading