From 07f7c15316383a647cff408ca536e66713f496e1 Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Mon, 10 Aug 2026 13:10:07 -0400 Subject: [PATCH 1/7] feat: add --manifest-source flag to run and deploy commands --- cmd/platform/deploy.go | 4 +++ cmd/platform/run.go | 5 ++- internal/cmdutil/flags.go | 25 +++++++++++++ internal/cmdutil/flags_test.go | 45 ++++++++++++++++++++++++ internal/config/config.go | 1 + internal/manifest/sync.go | 8 ++--- internal/manifest/sync_test.go | 64 ++++++++++++++++++++++++++++++++-- 7 files changed, 145 insertions(+), 7 deletions(-) diff --git a/cmd/platform/deploy.go b/cmd/platform/deploy.go index 88eed572..b96cde4c 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 (project or remote)") cmd.Flags().StringVar(&deployFlags.orgGrantWorkspaceID, cmdutil.OrgGrantWorkspaceFlag, "", cmdutil.OrgGrantWorkspaceDescription()) return cmd diff --git a/cmd/platform/run.go b/cmd/platform/run.go index 35d258fa..b887ceed 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 (project 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/internal/cmdutil/flags.go b/internal/cmdutil/flags.go index cdbea12a..572edf2e 100644 --- a/internal/cmdutil/flags.go +++ b/internal/cmdutil/flags.go @@ -17,6 +17,8 @@ package cmdutil import ( "fmt" + "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 +37,29 @@ var OrgGrantWorkspaceDescription = func() string { style.Secondary("(or 'all' for all workspaces in the org)")) } +// ManifestSourceFlag values +const ( + ManifestSourceProject = "project" + ManifestSourceRemote = "remote" +) + +// 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 != ManifestSourceProject && v != 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(ManifestSourceProject), + style.Highlight(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..7dab9bd1 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 + }{ + "empty string is valid": { + value: "", + expectErr: false, + }, + "project is valid": { + value: "project", + expectErr: false, + }, + "remote is valid": { + value: "remote", + expectErr: false, + }, + "invalid value returns error": { + value: "invalid", + expectErr: true, + }, + "local is not valid": { + value: "local", + 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..e1112664 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -54,6 +54,7 @@ type Config struct { DisableTelemetryFlag bool ForceFlag bool ForceRemoteFlag bool + ManifestSourceFlag string LogstashHostResolved string NoColor bool RuntimeFlag string diff --git a/internal/manifest/sync.go b/internal/manifest/sync.go index feb30e15..bc7246a4 100644 --- a/internal/manifest/sync.go +++ b/internal/manifest/sync.go @@ -77,12 +77,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 == "project" || 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 == "remote" || clients.Config.ForceRemoteFlag: merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllRemote) if err != nil { return nil, err @@ -91,8 +91,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=project"), + 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 a9feeb6c..63be4859 100644 --- a/internal/manifest/sync_test.go +++ b/internal/manifest/sync_test.go @@ -220,6 +220,66 @@ func Test_Sync(t *testing.T) { assert.Equal(t, "Remote", result.Merged.DisplayInformation.Description) }) + t.Run("manifest-source=project 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 = "project" + 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 = "remote" + 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("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=project") + 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) @@ -279,7 +339,7 @@ func Test_Sync(t *testing.T) { assert.Contains(t, err.Error(), "cache") }) - t.Run("missing manifest.json still succeeds with warning", func(t *testing.T) { + t.Run("missing manifest.json creates the file", 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). @@ -297,7 +357,7 @@ func Test_Sync(t *testing.T) { require.NoError(t, err) require.NotNil(t, result) assert.True(t, result.HasDifferences) - assert.False(t, result.WriteBack.Written) + assert.True(t, result.WriteBack.Written) }) t.Run("TTY interactive resolution with all-local strategy", func(t *testing.T) { From 159f342ad4d95a6be1be37cedfc8a4ed650e8211 Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Mon, 10 Aug 2026 13:10:45 -0400 Subject: [PATCH 2/7] feat: prefer manifest.json over get-manifest hook for local manifest reads --- internal/app/app.go | 2 +- internal/app/manifest.go | 35 ++++- internal/app/manifest_test.go | 220 ++++++++++++++++++---------- internal/manifest/writeback.go | 13 +- internal/manifest/writeback_test.go | 11 +- 5 files changed, 196 insertions(+), 85 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 2f5836a6..6a17a5cb 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -41,7 +41,7 @@ func NewClient( os types.Os, ) *Client { return &Client{ - Manifest: NewManifestClient(apiClient, config), + Manifest: NewManifestClient(apiClient, config, fs), AppClientInterface: NewAppClient(config, fs, os), } } diff --git a/internal/app/manifest.go b/internal/app/manifest.go index 2dc96ab8..7d4f7b48 100644 --- a/internal/app/manifest.go +++ b/internal/app/manifest.go @@ -17,6 +17,7 @@ package app import ( "context" "encoding/json" + "path/filepath" "strings" "github.com/slackapi/slack-cli/internal/api" @@ -24,11 +25,15 @@ import ( "github.com/slackapi/slack-cli/internal/hooks" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" + "github.com/spf13/afero" ) +const manifestFileName = "manifest.json" + // ManifestClient can manage the state of the project's app manifest file type ManifestClient struct { apiClient api.APIInterface + fs afero.Fs domainAuthTokens string Env map[string]string } @@ -59,17 +64,44 @@ func SetManifestEnvTeamVars(manifestEnv map[string]string, appTeamDomain string, func NewManifestClient( apiClient api.APIInterface, config *config.Config, + fs afero.Fs, ) *ManifestClient { client := &ManifestClient{ apiClient: apiClient, + fs: fs, domainAuthTokens: config.DomainAuthTokens, Env: config.ManifestEnv, } return client } -// GetManifestLocal gathers manifest content from the "get-manifest" hook +// GetManifestLocal reads the local manifest, preferring a static manifest.json +// file in the project root. Falls back to the "get-manifest" hook when no file exists. func (c *ManifestClient) GetManifestLocal(ctx context.Context, sdkConfig hooks.SDKCLIConfig, hookExecutor hooks.HookExecutor) (types.SlackYaml, error) { + manifestPath := filepath.Join(sdkConfig.WorkingDirectory, manifestFileName) + if exists, _ := afero.Exists(c.fs, manifestPath); exists { + return c.readManifestFile(manifestPath) + } + return c.getManifestFromHook(ctx, sdkConfig, hookExecutor) +} + +func (c *ManifestClient) readManifestFile(path string) (types.SlackYaml, error) { + var sl types.SlackYaml + data, err := afero.ReadFile(c.fs, path) + if err != nil { + return sl, slackerror.New("Failed to read manifest file"). + WithRootCause(err). + WithCode(slackerror.ErrInvalidManifest) + } + if err := json.Unmarshal(data, &sl); err != nil { + return sl, slackerror.New("Failed to parse manifest file"). + WithRootCause(err). + WithCode(slackerror.ErrInvalidManifest) + } + return sl, nil +} + +func (c *ManifestClient) getManifestFromHook(ctx context.Context, sdkConfig hooks.SDKCLIConfig, hookExecutor hooks.HookExecutor) (types.SlackYaml, error) { var sl types.SlackYaml if !sdkConfig.Hooks.GetManifest.IsAvailable() { @@ -104,7 +136,6 @@ func (c *ManifestClient) GetManifestLocal(ctx context.Context, sdkConfig hooks.S if start != -1 { slackManifestInfo = slackManifestInfo[start:] } else { - // the app manifest has to be a json so needs to have the character `{` return sl, slackerror.New("Invalid app manifest format, must be valid JSON"). WithRootCause(err). WithCode(slackerror.ErrInvalidManifest) diff --git a/internal/app/manifest_test.go b/internal/app/manifest_test.go index 41184e74..2f39281a 100644 --- a/internal/app/manifest_test.go +++ b/internal/app/manifest_test.go @@ -24,6 +24,7 @@ import ( "github.com/slackapi/slack-cli/internal/slackcontext" "github.com/slackapi/slack-cli/internal/slackdeps" "github.com/slackapi/slack-cli/internal/slackerror" + "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -68,80 +69,149 @@ func Test_AppManifest_SetManifestEnvTeamVars(t *testing.T) { } func Test_AppManifest_GetManifestLocal(t *testing.T) { - tests := map[string]struct { - mockManifestInfo string - mockManifestErr error - expectedErr error - expectedManifest types.SlackYaml - }{ - "errors if no get-manifest hook exists": { - expectedErr: slackerror.New(slackerror.ErrSDKHookNotFound), - }, - "returns an existing manifest without errors": { - mockManifestInfo: `{"display_information":{"name":"my-example-app"}}`, - expectedManifest: types.SlackYaml{ - AppManifest: types.AppManifest{ - DisplayInformation: types.DisplayInformation{ - Name: "my-example-app", - }, - }, - }, - }, - "errors if the hook execution errors": { - mockManifestInfo: `{}`, - mockManifestErr: slackerror.New(slackerror.ErrNoFile), - expectedErr: slackerror.New(slackerror.ErrInvalidManifest), - }, - "parses a manifest with random leading characters": { - mockManifestInfo: `...{"display_information":{"name":"my-showcased-app"}}`, - expectedManifest: types.SlackYaml{ - AppManifest: types.AppManifest{ - DisplayInformation: types.DisplayInformation{ - Name: "my-showcased-app", - }, - }, - }, - }, - "errors if a manifest is not present in output": { - mockManifestInfo: `...unknown`, - expectedErr: slackerror.New(slackerror.ErrInvalidManifest), - }, - } - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - ctx := slackcontext.MockContext(t.Context()) - mockManifestEnv := map[string]string{"EXAMPLE": "12"} - mockSDKConfig := hooks.NewSDKConfigMock() - mockHookExecutor := &hooks.MockHookExecutor{} - if tc.mockManifestInfo != "" { - mockSDKConfig.Hooks.GetManifest = hooks.HookScript{ - Name: "GetManifest", - Command: "cat manifest.json", - } - mockHookExecutor.On("Execute", mock.Anything, mock.Anything). - Return(tc.mockManifestInfo, tc.mockManifestErr) - } else { - mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest"} - } - fsMock := slackdeps.NewFsMock() - osMock := slackdeps.NewOsMock() - osMock.AddDefaultMocks() - configMock := config.NewConfig(fsMock, osMock) - configMock.DomainAuthTokens = "api.slack.com" - configMock.ManifestEnv = mockManifestEnv - manifestClient := NewManifestClient(&api.APIMock{}, configMock) - - actualManifest, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) - if tc.expectedErr != nil { - require.Error(t, err) - assert.Equal(t, - tc.expectedErr.(*slackerror.Error).Code, err.(*slackerror.Error).Code) - } else { - require.NoError(t, err) - assert.Equal(t, tc.expectedManifest, actualManifest) - } - }) - } + t.Run("reads manifest.json directly when it exists", func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.WorkingDirectory = "/project" + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "echo manifest"} + + _ = fsMock.MkdirAll("/project", 0755) + _ = afero.WriteFile(fsMock, "/project/manifest.json", []byte(`{"display_information":{"name":"file-app"}}`), 0644) + + mockHookExecutor := &hooks.MockHookExecutor{} + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + + result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + require.NoError(t, err) + assert.Equal(t, "file-app", result.DisplayInformation.Name) + mockHookExecutor.AssertNotCalled(t, "Execute", mock.Anything, mock.Anything) + }) + + t.Run("falls back to get-manifest hook when no manifest.json exists", func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + configMock.DomainAuthTokens = "api.slack.com" + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} + + mockHookExecutor := &hooks.MockHookExecutor{} + mockHookExecutor.On("Execute", mock.Anything, mock.Anything). + Return(`{"display_information":{"name":"hook-app"}}`, nil) + + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + + result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + require.NoError(t, err) + assert.Equal(t, "hook-app", result.DisplayInformation.Name) + mockHookExecutor.AssertCalled(t, "Execute", mock.Anything, mock.Anything) + }) + + t.Run("errors if no manifest.json and no get-manifest hook exists", func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest"} + + mockHookExecutor := &hooks.MockHookExecutor{} + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + + _, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + require.Error(t, err) + assert.Equal(t, slackerror.ErrSDKHookNotFound, err.(*slackerror.Error).Code) + }) + + t.Run("errors if manifest.json contains invalid JSON", func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.WorkingDirectory = "/project" + + _ = fsMock.MkdirAll("/project", 0755) + _ = afero.WriteFile(fsMock, "/project/manifest.json", []byte(`not json`), 0644) + + mockHookExecutor := &hooks.MockHookExecutor{} + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + + _, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + require.Error(t, err) + assert.Equal(t, slackerror.ErrInvalidManifest, err.(*slackerror.Error).Code) + }) + + t.Run("errors if hook execution errors", func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + configMock.DomainAuthTokens = "api.slack.com" + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} + + mockHookExecutor := &hooks.MockHookExecutor{} + mockHookExecutor.On("Execute", mock.Anything, mock.Anything). + Return(`{}`, slackerror.New(slackerror.ErrNoFile)) + + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + + _, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + require.Error(t, err) + assert.Equal(t, slackerror.ErrInvalidManifest, err.(*slackerror.Error).Code) + }) + + t.Run("parses hook output with random leading characters", func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + configMock.DomainAuthTokens = "api.slack.com" + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} + + mockHookExecutor := &hooks.MockHookExecutor{} + mockHookExecutor.On("Execute", mock.Anything, mock.Anything). + Return(`...{"display_information":{"name":"hook-app"}}`, nil) + + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + + result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + require.NoError(t, err) + assert.Equal(t, "hook-app", result.DisplayInformation.Name) + }) + + t.Run("errors if hook output has no JSON", func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + configMock.DomainAuthTokens = "api.slack.com" + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} + + mockHookExecutor := &hooks.MockHookExecutor{} + mockHookExecutor.On("Execute", mock.Anything, mock.Anything). + Return(`...unknown`, nil) + + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + + _, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + require.Error(t, err) + assert.Equal(t, slackerror.ErrInvalidManifest, err.(*slackerror.Error).Code) + }) } func Test_AppManifest_GetManifestRemote(t *testing.T) { @@ -186,7 +256,7 @@ func Test_AppManifest_GetManifestRemote(t *testing.T) { apic := &api.APIMock{} apic.On("ExportAppManifest", mock.Anything, mock.Anything, mock.Anything). Return(api.ExportAppResult{Manifest: tc.mockManifestResponse}, tc.mockManifestError) - manifestClient := NewManifestClient(apic, configMock) + manifestClient := NewManifestClient(apic, configMock, fsMock) manifest, err := manifestClient.GetManifestRemote(ctx, tc.mockToken, tc.mockAppID) if tc.expectedError != nil { diff --git a/internal/manifest/writeback.go b/internal/manifest/writeback.go index 725b2208..e5b65c32 100644 --- a/internal/manifest/writeback.go +++ b/internal/manifest/writeback.go @@ -36,7 +36,7 @@ type WriteBackResult struct { // WriteManifestLocal writes the merged manifest back to the project's // manifest.json file, preserving the original file's key ordering by -// using the same JSON structure. +// using the same JSON structure. Creates the file if it does not exist. func WriteManifestLocal(fs afero.Fs, workingDir string, manifest types.AppManifest) (WriteBackResult, error) { manifestPath := filepath.Join(workingDir, manifestFileName) @@ -45,9 +45,14 @@ func WriteManifestLocal(fs afero.Fs, workingDir string, manifest types.AppManife return WriteBackResult{}, fmt.Errorf("failed to check manifest file: %w", err) } if !exists { - return WriteBackResult{ - Warning: fmt.Sprintf("No %s found in project root — merged manifest was not written locally", manifestFileName), - }, nil + fresh, err := marshalFresh(manifest) + if err != nil { + return WriteBackResult{}, fmt.Errorf("failed to serialize merged manifest: %w", err) + } + if err := atomicWriteFile(fs, manifestPath, fresh, 0644); err != nil { + return WriteBackResult{}, fmt.Errorf("failed to write %s: %w", manifestFileName, err) + } + return WriteBackResult{Written: true, FilePath: manifestPath}, nil } original, err := afero.ReadFile(fs, manifestPath) diff --git a/internal/manifest/writeback_test.go b/internal/manifest/writeback_test.go index 475327af..bfaa36a7 100644 --- a/internal/manifest/writeback_test.go +++ b/internal/manifest/writeback_test.go @@ -110,16 +110,21 @@ func Test_WriteManifestLocal(t *testing.T) { assert.Contains(t, result.Warning, "key order was not preserved") }) - t.Run("returns warning when manifest.json does not exist", func(t *testing.T) { + t.Run("creates manifest.json when it does not exist", func(t *testing.T) { fs := afero.NewMemMapFs() + _ = fs.MkdirAll("/project", 0755) manifest := types.AppManifest{ DisplayInformation: types.DisplayInformation{Name: "App"}, } result, err := WriteManifestLocal(fs, "/project", manifest) require.NoError(t, err) - assert.False(t, result.Written) - assert.Contains(t, result.Warning, "No manifest.json found") + assert.True(t, result.Written) + assert.Equal(t, "/project/manifest.json", result.FilePath) + + content, err := afero.ReadFile(fs, "/project/manifest.json") + require.NoError(t, err) + assert.Contains(t, string(content), `"name": "App"`) }) } From 4ed20686a70c9842c49f31de9580c2cb23da9e04 Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Mon, 10 Aug 2026 13:14:36 -0400 Subject: [PATCH 3/7] test: rename misleading test case for manifest-source flag validation --- internal/cmdutil/flags_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cmdutil/flags_test.go b/internal/cmdutil/flags_test.go index 7dab9bd1..7bfc6498 100644 --- a/internal/cmdutil/flags_test.go +++ b/internal/cmdutil/flags_test.go @@ -29,7 +29,7 @@ func Test_ValidateManifestSourceFlag(t *testing.T) { value string expectErr bool }{ - "empty string is valid": { + "flag not provided is valid": { value: "", expectErr: false, }, From 48bf21afea8bfa0957a939fc379924fa1e39a96a Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Mon, 10 Aug 2026 13:16:37 -0400 Subject: [PATCH 4/7] refactor: consolidate repetitive hook fallback tests into table-driven test --- internal/app/manifest_test.go | 129 +++++++++++++--------------------- 1 file changed, 48 insertions(+), 81 deletions(-) diff --git a/internal/app/manifest_test.go b/internal/app/manifest_test.go index 2f39281a..23be5430 100644 --- a/internal/app/manifest_test.go +++ b/internal/app/manifest_test.go @@ -91,28 +91,6 @@ func Test_AppManifest_GetManifestLocal(t *testing.T) { mockHookExecutor.AssertNotCalled(t, "Execute", mock.Anything, mock.Anything) }) - t.Run("falls back to get-manifest hook when no manifest.json exists", func(t *testing.T) { - ctx := slackcontext.MockContext(t.Context()) - fsMock := slackdeps.NewFsMock() - osMock := slackdeps.NewOsMock() - osMock.AddDefaultMocks() - configMock := config.NewConfig(fsMock, osMock) - configMock.DomainAuthTokens = "api.slack.com" - mockSDKConfig := hooks.NewSDKConfigMock() - mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} - - mockHookExecutor := &hooks.MockHookExecutor{} - mockHookExecutor.On("Execute", mock.Anything, mock.Anything). - Return(`{"display_information":{"name":"hook-app"}}`, nil) - - manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) - - result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) - require.NoError(t, err) - assert.Equal(t, "hook-app", result.DisplayInformation.Name) - mockHookExecutor.AssertCalled(t, "Execute", mock.Anything, mock.Anything) - }) - t.Run("errors if no manifest.json and no get-manifest hook exists", func(t *testing.T) { ctx := slackcontext.MockContext(t.Context()) fsMock := slackdeps.NewFsMock() @@ -150,68 +128,57 @@ func Test_AppManifest_GetManifestLocal(t *testing.T) { assert.Equal(t, slackerror.ErrInvalidManifest, err.(*slackerror.Error).Code) }) - t.Run("errors if hook execution errors", func(t *testing.T) { - ctx := slackcontext.MockContext(t.Context()) - fsMock := slackdeps.NewFsMock() - osMock := slackdeps.NewOsMock() - osMock.AddDefaultMocks() - configMock := config.NewConfig(fsMock, osMock) - configMock.DomainAuthTokens = "api.slack.com" - mockSDKConfig := hooks.NewSDKConfigMock() - mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} - - mockHookExecutor := &hooks.MockHookExecutor{} - mockHookExecutor.On("Execute", mock.Anything, mock.Anything). - Return(`{}`, slackerror.New(slackerror.ErrNoFile)) - - manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) - - _, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) - require.Error(t, err) - assert.Equal(t, slackerror.ErrInvalidManifest, err.(*slackerror.Error).Code) - }) - - t.Run("parses hook output with random leading characters", func(t *testing.T) { - ctx := slackcontext.MockContext(t.Context()) - fsMock := slackdeps.NewFsMock() - osMock := slackdeps.NewOsMock() - osMock.AddDefaultMocks() - configMock := config.NewConfig(fsMock, osMock) - configMock.DomainAuthTokens = "api.slack.com" - mockSDKConfig := hooks.NewSDKConfigMock() - mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} - - mockHookExecutor := &hooks.MockHookExecutor{} - mockHookExecutor.On("Execute", mock.Anything, mock.Anything). - Return(`...{"display_information":{"name":"hook-app"}}`, nil) - - manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) - - result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) - require.NoError(t, err) - assert.Equal(t, "hook-app", result.DisplayInformation.Name) - }) - - t.Run("errors if hook output has no JSON", func(t *testing.T) { - ctx := slackcontext.MockContext(t.Context()) - fsMock := slackdeps.NewFsMock() - osMock := slackdeps.NewOsMock() - osMock.AddDefaultMocks() - configMock := config.NewConfig(fsMock, osMock) - configMock.DomainAuthTokens = "api.slack.com" - mockSDKConfig := hooks.NewSDKConfigMock() - mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} + hookFallbackTests := map[string]struct { + hookOutput string + hookErr error + expectedName string + expectedErr string + }{ + "falls back to hook when no manifest.json exists": { + hookOutput: `{"display_information":{"name":"hook-app"}}`, + expectedName: "hook-app", + }, + "parses hook output with leading characters": { + hookOutput: `...{"display_information":{"name":"hook-app"}}`, + expectedName: "hook-app", + }, + "errors if hook execution errors": { + hookOutput: `{}`, + hookErr: slackerror.New(slackerror.ErrNoFile), + expectedErr: slackerror.ErrInvalidManifest, + }, + "errors if hook output has no JSON": { + hookOutput: `...unknown`, + expectedErr: slackerror.ErrInvalidManifest, + }, + } + for name, tc := range hookFallbackTests { + t.Run(name, func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + configMock.DomainAuthTokens = "api.slack.com" + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} - mockHookExecutor := &hooks.MockHookExecutor{} - mockHookExecutor.On("Execute", mock.Anything, mock.Anything). - Return(`...unknown`, nil) + mockHookExecutor := &hooks.MockHookExecutor{} + mockHookExecutor.On("Execute", mock.Anything, mock.Anything). + Return(tc.hookOutput, tc.hookErr) - manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) - _, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) - require.Error(t, err) - assert.Equal(t, slackerror.ErrInvalidManifest, err.(*slackerror.Error).Code) - }) + result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + if tc.expectedErr != "" { + require.Error(t, err) + assert.Equal(t, tc.expectedErr, err.(*slackerror.Error).Code) + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedName, result.DisplayInformation.Name) + } + }) + } } func Test_AppManifest_GetManifestRemote(t *testing.T) { From b2d74edae9603e04e5fc9bf1b4539c3cf0e22fac Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Mon, 10 Aug 2026 13:17:28 -0400 Subject: [PATCH 5/7] refactor: consolidate sync merge strategy tests into table-driven test --- internal/manifest/sync_test.go | 139 ++++++++++++--------------------- 1 file changed, 49 insertions(+), 90 deletions(-) diff --git a/internal/manifest/sync_test.go b/internal/manifest/sync_test.go index 63be4859..4b3a4808 100644 --- a/internal/manifest/sync_test.go +++ b/internal/manifest/sync_test.go @@ -173,96 +173,55 @@ func Test_Sync(t *testing.T) { assert.Equal(t, slackerror.ErrAppManifestUpdate, slackErr.Code) }) - t.Run("force flag 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.ForceFlag = true - 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.True(t, result.WriteBack.Written) - f.clientsMock.API.AssertCalled(t, "UpdateApp", mock.Anything, "xoxb-test", "A123", mock.Anything, true, true) - }) - - t.Run("force-remote flag 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.ForceRemoteFlag = true - 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.True(t, result.WriteBack.Written) - // Verify remote value was used — the merged manifest should have "Remote" description - assert.Equal(t, "Remote", result.Merged.DisplayInformation.Description) - }) - - t.Run("manifest-source=project 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 = "project" - 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 = "remote" - 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) - }) + mergeStrategyTests := map[string]struct { + forceFlag bool + forceRemoteFlag bool + manifestSourceFlag string + expectedDesc string + }{ + "force flag merges all local": { + forceFlag: true, + expectedDesc: "Local", + }, + "force-remote flag merges all remote": { + forceRemoteFlag: true, + expectedDesc: "Remote", + }, + "manifest-source=project merges all local": { + manifestSourceFlag: "project", + expectedDesc: "Local", + }, + "manifest-source=remote merges all remote": { + manifestSourceFlag: "remote", + expectedDesc: "Remote", + }, + } + for name, tc := range mergeStrategyTests { + t.Run(name, 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.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) + 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.True(t, result.WriteBack.Written) + assert.Equal(t, tc.expectedDesc, result.Merged.DisplayInformation.Description) + }) + } t.Run("non-TTY error mentions --manifest-source in remediation", func(t *testing.T) { f := newSyncTestFixture(t) From 9a27637826a895bec43e39abc02c75caa6f467cc Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Mon, 10 Aug 2026 13:55:43 -0400 Subject: [PATCH 6/7] revert: remove --manifest-source flag (out of scope for this PR) --- cmd/platform/deploy.go | 4 --- cmd/platform/run.go | 4 --- internal/cmdutil/flags.go | 25 ------------------- internal/cmdutil/flags_test.go | 45 ---------------------------------- internal/config/config.go | 1 - internal/manifest/sync.go | 8 +++--- internal/manifest/sync_test.go | 32 +++--------------------- 7 files changed, 7 insertions(+), 112 deletions(-) diff --git a/cmd/platform/deploy.go b/cmd/platform/deploy.go index b96cde4c..88eed572 100644 --- a/cmd/platform/deploy.go +++ b/cmd/platform/deploy.go @@ -59,9 +59,6 @@ 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 { @@ -111,7 +108,6 @@ 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 (project or remote)") cmd.Flags().StringVar(&deployFlags.orgGrantWorkspaceID, cmdutil.OrgGrantWorkspaceFlag, "", cmdutil.OrgGrantWorkspaceDescription()) return cmd diff --git a/cmd/platform/run.go b/cmd/platform/run.go index b887ceed..61cd5244 100644 --- a/cmd/platform/run.go +++ b/cmd/platform/run.go @@ -58,9 +58,6 @@ 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 { - if err := cmdutil.ValidateManifestSourceFlag(clients); err != nil { - return err - } return cmdutil.IsValidProjectDirectory(clients) }, RunE: func(cmd *cobra.Command, args []string) error { @@ -72,7 +69,6 @@ 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 (project 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/internal/cmdutil/flags.go b/internal/cmdutil/flags.go index 572edf2e..cdbea12a 100644 --- a/internal/cmdutil/flags.go +++ b/internal/cmdutil/flags.go @@ -17,8 +17,6 @@ package cmdutil import ( "fmt" - "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" ) @@ -37,29 +35,6 @@ var OrgGrantWorkspaceDescription = func() string { style.Secondary("(or 'all' for all workspaces in the org)")) } -// ManifestSourceFlag values -const ( - ManifestSourceProject = "project" - ManifestSourceRemote = "remote" -) - -// 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 != ManifestSourceProject && v != 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(ManifestSourceProject), - style.Highlight(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 7bfc6498..2189bc95 100644 --- a/internal/cmdutil/flags_test.go +++ b/internal/cmdutil/flags_test.go @@ -17,55 +17,10 @@ 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, - }, - "project is valid": { - value: "project", - expectErr: false, - }, - "remote is valid": { - value: "remote", - expectErr: false, - }, - "invalid value returns error": { - value: "invalid", - expectErr: true, - }, - "local is not valid": { - value: "local", - 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 e1112664..979a0afe 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -54,7 +54,6 @@ type Config struct { DisableTelemetryFlag bool ForceFlag bool ForceRemoteFlag bool - ManifestSourceFlag string LogstashHostResolved string NoColor bool RuntimeFlag string diff --git a/internal/manifest/sync.go b/internal/manifest/sync.go index bc7246a4..feb30e15 100644 --- a/internal/manifest/sync.go +++ b/internal/manifest/sync.go @@ -77,12 +77,12 @@ func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, aut var merged types.AppManifest switch { - case clients.Config.ManifestSourceFlag == "project" || clients.Config.ForceFlag: + case clients.Config.ForceFlag: merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllLocal) if err != nil { return nil, err } - case clients.Config.ManifestSourceFlag == "remote" || clients.Config.ForceRemoteFlag: + case clients.Config.ForceRemoteFlag: merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllRemote) if err != nil { return nil, err @@ -91,8 +91,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("--manifest-source=project"), - style.CommandText("--manifest-source=remote"), + style.CommandText("--force"), + style.CommandText("--force-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 4b3a4808..e0700676 100644 --- a/internal/manifest/sync_test.go +++ b/internal/manifest/sync_test.go @@ -174,10 +174,9 @@ func Test_Sync(t *testing.T) { }) mergeStrategyTests := map[string]struct { - forceFlag bool - forceRemoteFlag bool - manifestSourceFlag string - expectedDesc string + forceFlag bool + forceRemoteFlag bool + expectedDesc string }{ "force flag merges all local": { forceFlag: true, @@ -187,14 +186,6 @@ func Test_Sync(t *testing.T) { forceRemoteFlag: true, expectedDesc: "Remote", }, - "manifest-source=project merges all local": { - manifestSourceFlag: "project", - expectedDesc: "Local", - }, - "manifest-source=remote merges all remote": { - manifestSourceFlag: "remote", - expectedDesc: "Remote", - }, } for name, tc := range mergeStrategyTests { t.Run(name, func(t *testing.T) { @@ -206,7 +197,6 @@ func Test_Sync(t *testing.T) { 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) @@ -223,22 +213,6 @@ func Test_Sync(t *testing.T) { }) } - 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=project") - 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) From da81af04e688eea4542b1b62d756085f3e4fe105 Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Tue, 11 Aug 2026 14:57:40 -0400 Subject: [PATCH 7/7] fix: prefer get-manifest hook over manifest.json with file as fallback --- internal/app/manifest.go | 12 +++++------ internal/app/manifest_test.go | 39 +++++++++++++++++++++++++++++------ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/internal/app/manifest.go b/internal/app/manifest.go index 7d4f7b48..a6d95a2c 100644 --- a/internal/app/manifest.go +++ b/internal/app/manifest.go @@ -75,14 +75,14 @@ func NewManifestClient( return client } -// GetManifestLocal reads the local manifest, preferring a static manifest.json -// file in the project root. Falls back to the "get-manifest" hook when no file exists. +// GetManifestLocal reads the local manifest, preferring the "get-manifest" hook +// when available. Falls back to reading manifest.json directly from the project root. func (c *ManifestClient) GetManifestLocal(ctx context.Context, sdkConfig hooks.SDKCLIConfig, hookExecutor hooks.HookExecutor) (types.SlackYaml, error) { - manifestPath := filepath.Join(sdkConfig.WorkingDirectory, manifestFileName) - if exists, _ := afero.Exists(c.fs, manifestPath); exists { - return c.readManifestFile(manifestPath) + if sdkConfig.Hooks.GetManifest.IsAvailable() { + return c.getManifestFromHook(ctx, sdkConfig, hookExecutor) } - return c.getManifestFromHook(ctx, sdkConfig, hookExecutor) + manifestPath := filepath.Join(sdkConfig.WorkingDirectory, manifestFileName) + return c.readManifestFile(manifestPath) } func (c *ManifestClient) readManifestFile(path string) (types.SlackYaml, error) { diff --git a/internal/app/manifest_test.go b/internal/app/manifest_test.go index 23be5430..3cdc03d7 100644 --- a/internal/app/manifest_test.go +++ b/internal/app/manifest_test.go @@ -69,12 +69,13 @@ func Test_AppManifest_SetManifestEnvTeamVars(t *testing.T) { } func Test_AppManifest_GetManifestLocal(t *testing.T) { - t.Run("reads manifest.json directly when it exists", func(t *testing.T) { + t.Run("uses hook when get-manifest is available", func(t *testing.T) { ctx := slackcontext.MockContext(t.Context()) fsMock := slackdeps.NewFsMock() osMock := slackdeps.NewOsMock() osMock.AddDefaultMocks() configMock := config.NewConfig(fsMock, osMock) + configMock.DomainAuthTokens = "api.slack.com" mockSDKConfig := hooks.NewSDKConfigMock() mockSDKConfig.WorkingDirectory = "/project" mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "echo manifest"} @@ -82,6 +83,30 @@ func Test_AppManifest_GetManifestLocal(t *testing.T) { _ = fsMock.MkdirAll("/project", 0755) _ = afero.WriteFile(fsMock, "/project/manifest.json", []byte(`{"display_information":{"name":"file-app"}}`), 0644) + mockHookExecutor := &hooks.MockHookExecutor{} + mockHookExecutor.On("Execute", mock.Anything, mock.Anything). + Return(`{"display_information":{"name":"hook-app"}}`, nil) + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) + + result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + require.NoError(t, err) + assert.Equal(t, "hook-app", result.DisplayInformation.Name) + mockHookExecutor.AssertCalled(t, "Execute", mock.Anything, mock.Anything) + }) + + t.Run("falls back to manifest.json when no hook exists", func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fsMock := slackdeps.NewFsMock() + osMock := slackdeps.NewOsMock() + osMock.AddDefaultMocks() + configMock := config.NewConfig(fsMock, osMock) + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.WorkingDirectory = "/project" + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest"} + + _ = fsMock.MkdirAll("/project", 0755) + _ = afero.WriteFile(fsMock, "/project/manifest.json", []byte(`{"display_information":{"name":"file-app"}}`), 0644) + mockHookExecutor := &hooks.MockHookExecutor{} manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) @@ -91,13 +116,14 @@ func Test_AppManifest_GetManifestLocal(t *testing.T) { mockHookExecutor.AssertNotCalled(t, "Execute", mock.Anything, mock.Anything) }) - t.Run("errors if no manifest.json and no get-manifest hook exists", func(t *testing.T) { + t.Run("errors if no hook and no manifest.json", func(t *testing.T) { ctx := slackcontext.MockContext(t.Context()) fsMock := slackdeps.NewFsMock() osMock := slackdeps.NewOsMock() osMock.AddDefaultMocks() configMock := config.NewConfig(fsMock, osMock) mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.WorkingDirectory = "/project" mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest"} mockHookExecutor := &hooks.MockHookExecutor{} @@ -105,7 +131,7 @@ func Test_AppManifest_GetManifestLocal(t *testing.T) { _, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) require.Error(t, err) - assert.Equal(t, slackerror.ErrSDKHookNotFound, err.(*slackerror.Error).Code) + assert.Equal(t, slackerror.ErrInvalidManifest, err.(*slackerror.Error).Code) }) t.Run("errors if manifest.json contains invalid JSON", func(t *testing.T) { @@ -116,6 +142,7 @@ func Test_AppManifest_GetManifestLocal(t *testing.T) { configMock := config.NewConfig(fsMock, osMock) mockSDKConfig := hooks.NewSDKConfigMock() mockSDKConfig.WorkingDirectory = "/project" + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest"} _ = fsMock.MkdirAll("/project", 0755) _ = afero.WriteFile(fsMock, "/project/manifest.json", []byte(`not json`), 0644) @@ -128,13 +155,13 @@ func Test_AppManifest_GetManifestLocal(t *testing.T) { assert.Equal(t, slackerror.ErrInvalidManifest, err.(*slackerror.Error).Code) }) - hookFallbackTests := map[string]struct { + hookTests := map[string]struct { hookOutput string hookErr error expectedName string expectedErr string }{ - "falls back to hook when no manifest.json exists": { + "returns manifest from hook output": { hookOutput: `{"display_information":{"name":"hook-app"}}`, expectedName: "hook-app", }, @@ -152,7 +179,7 @@ func Test_AppManifest_GetManifestLocal(t *testing.T) { expectedErr: slackerror.ErrInvalidManifest, }, } - for name, tc := range hookFallbackTests { + for name, tc := range hookTests { t.Run(name, func(t *testing.T) { ctx := slackcontext.MockContext(t.Context()) fsMock := slackdeps.NewFsMock()