diff --git a/cmd/platform/run.go b/cmd/platform/run.go index 35d258fa..61cd5244 100644 --- a/cmd/platform/run.go +++ b/cmd/platform/run.go @@ -58,7 +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 { - // Verify command is run in a project directory return cmdutil.IsValidProjectDirectory(clients) }, RunE: func(cmd *cobra.Command, args []string) error { 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..a6d95a2c 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 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) { + if sdkConfig.Hooks.GetManifest.IsAvailable() { + 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) { + 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..3cdc03d7 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,77 +69,140 @@ 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 + 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"} + + _ = 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) + + 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("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{} + 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 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" + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest"} + + _ = 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) + }) + + hookTests := map[string]struct { + hookOutput string + hookErr error + expectedName string + expectedErr string }{ - "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", - }, - }, - }, + "returns manifest from hook output": { + hookOutput: `{"display_information":{"name":"hook-app"}}`, + expectedName: "hook-app", }, - "errors if the hook execution errors": { - mockManifestInfo: `{}`, - mockManifestErr: slackerror.New(slackerror.ErrNoFile), - expectedErr: slackerror.New(slackerror.ErrInvalidManifest), + "parses hook output with leading characters": { + hookOutput: `...{"display_information":{"name":"hook-app"}}`, + expectedName: "hook-app", }, - "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 hook execution errors": { + hookOutput: `{}`, + hookErr: slackerror.New(slackerror.ErrNoFile), + expectedErr: slackerror.ErrInvalidManifest, }, - "errors if a manifest is not present in output": { - mockManifestInfo: `...unknown`, - expectedErr: slackerror.New(slackerror.ErrInvalidManifest), + "errors if hook output has no JSON": { + hookOutput: `...unknown`, + expectedErr: slackerror.ErrInvalidManifest, }, } - for name, tc := range tests { + for name, tc := range hookTests { 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) + mockSDKConfig := hooks.NewSDKConfigMock() + mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"} + + mockHookExecutor := &hooks.MockHookExecutor{} + mockHookExecutor.On("Execute", mock.Anything, mock.Anything). + Return(tc.hookOutput, tc.hookErr) + + manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock) - actualManifest, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) - if tc.expectedErr != nil { + result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor) + if tc.expectedErr != "" { require.Error(t, err) - assert.Equal(t, - tc.expectedErr.(*slackerror.Error).Code, err.(*slackerror.Error).Code) + assert.Equal(t, tc.expectedErr, err.(*slackerror.Error).Code) } else { require.NoError(t, err) - assert.Equal(t, tc.expectedManifest, actualManifest) + assert.Equal(t, tc.expectedName, result.DisplayInformation.Name) } }) } @@ -186,7 +250,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/sync_test.go b/internal/manifest/sync_test.go index a9feeb6c..e0700676 100644 --- a/internal/manifest/sync_test.go +++ b/internal/manifest/sync_test.go @@ -173,52 +173,45 @@ 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) - }) + mergeStrategyTests := map[string]struct { + forceFlag bool + forceRemoteFlag bool + expectedDesc string + }{ + "force flag merges all local": { + forceFlag: true, + expectedDesc: "Local", + }, + "force-remote flag merges all remote": { + forceRemoteFlag: true, + 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.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("API UpdateApp failure is propagated", func(t *testing.T) { f := newSyncTestFixture(t) @@ -279,7 +272,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 +290,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) { 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"`) }) }