Skip to content
Open
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
18 changes: 9 additions & 9 deletions cmd/app/link.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func LinkCommandRunE(ctx context.Context, clients *shared.ClientFactory, app *ty
// Add empty line between executed command and first output
clients.IO.PrintInfo(ctx, false, "")

err = LinkExistingApp(ctx, clients, app, false)
_, err = LinkExistingApp(ctx, clients, app, false)
if err != nil {
return err
}
Expand Down Expand Up @@ -130,7 +130,7 @@ func LinkAppHeaderSection(ctx context.Context, clients *shared.ClientFactory, sh
// When shouldConfirm is true, a confirmation prompt will ask the user if they want to
// link an existing app and additional information is included in the header.
// The shouldConfirm option is encouraged for third-party callers.
func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *types.App, shouldConfirm bool) (err error) {
func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *types.App, shouldConfirm bool) (_ *types.SlackAuth, err error) {

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.

suggestion: We should consider returning a single prompts.SelectedApp (contains an app and auth) instead of mixing a pointer out-parameter (app *types.App) with the new returned types.SlackAuth.

The app parameter is currently an "out-param" dressed up as an "in-param" - it's always overwritten at L171 (*app, auth, err = promptExistingApp(...)). Every caller passes a throwaway &types.App{}.

After this PR, app comes out via the pointer while auth comes out via the return value - two different output mechanisms for two values produced at the same call site. This makes the function confusing and brittle.

prompts.SelectedApp already pairs App + Auth and is the exact shape this function produces. Also, cmd/app already imports internal/prompts, so no new dependency.

Suggested signature:

func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, shouldConfirm bool) (prompts.SelectedApp, error)

A zero SelectedApp{} (i.e. selected.App.AppID == "") signals "user declined at the confirm prompt" - which is what create.go already checks on L243.

Caller changes:

  • LinkCommandRunE (this file): drop the app *types.App param; call _, err := LinkExistingApp(ctx, clients, false). Also remove the throwaway app := &types.App{} in NewLinkCommand.
  • cmd/project/init.go:113: _, err = app.LinkExistingApp(ctx, clients, true)
  • cmd/project/create.go:232: selected, linkErr := app.LinkExistingApp(ctx, clients, false) then if selected.App.AppID != "" { fetchAndWriteRemoteManifest(ctx, clients, selected.Auth.Token, selected.App.AppID, absProjectPath) } — the existing auth != nil && linkedApp.AppID != "" guard collapses to one condition.

Diff size is comparable to the current PR, no callers need to manufacture an empty App{}, and the API answers the obvious question: yes, this function returns a selected app.

// Header section
LinkAppHeaderSection(ctx, clients, shouldConfirm)

Expand All @@ -139,21 +139,21 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty
proceed, err := clients.IO.ConfirmPrompt(ctx, LinkAppConfirmPromptText, true)
if err != nil {
clients.IO.PrintDebug(ctx, "Error prompting to add an existing app: %s", err)
return err
return nil, err
}

// Add newline to match the trailing newline inserted from the footer section
clients.IO.PrintInfo(ctx, false, "")

if !proceed {
return nil
return nil, nil
}
}

// App Manifest section
manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx)
if err != nil {
return err
return nil, err
}

configPath := filepath.Join(config.ProjectConfigDirName, config.ProjectConfigJSONFilename)
Expand All @@ -170,26 +170,26 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty
var auth *types.SlackAuth
*app, auth, err = promptExistingApp(ctx, clients)
if err != nil {
return err
return nil, err
}

appIDs := []string{app.AppID}
_, err = clients.API().GetAppStatus(ctx, auth.Token, appIDs, app.TeamID)
if err != nil {
return err
return nil, err
}

// Save the app to the project
err = saveAppToJSON(ctx, clients, *app)
if err != nil {
clients.IO.PrintDebug(ctx, "Error saving app to file when linking existing app: %s", err)
return err
return nil, err
}

// Footer section
LinkAppFooterSection(ctx, clients, app)

return nil
return auth, nil

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.

🔗 question: Instead of returning an auth to calling functions that's often ignored, would setting up the manifest as part of the link command if no apps are saved make sense?

👾 ramble: I'm wanting to avoid mixing concepts with the create command which should be from a project template I think while link is seeming more app specific.

}

// LinkAppFooterSection displays the details of app that was added to the project.
Expand Down
37 changes: 35 additions & 2 deletions cmd/project/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package project

import (
"context"
"encoding/json"
"fmt"
"math/rand"
"os"
Expand All @@ -31,6 +32,7 @@ import (
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/slacktrace"
"github.com/slackapi/slack-cli/internal/style"
"github.com/spf13/afero"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -229,8 +231,24 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args []
defer func() {
_ = os.Chdir(originalDir)
}()
if err := app.LinkExistingApp(ctx, clients, &types.App{}, false); err != nil {
return err

linkedApp := &types.App{}
auth, linkErr := app.LinkExistingApp(ctx, clients, linkedApp, false)
if linkErr != nil {
return linkErr
}

if auth != nil && linkedApp.AppID != "" {
fetchErr := fetchAndWriteRemoteManifest(ctx, clients, auth.Token, linkedApp.AppID, absProjectPath)
if fetchErr != nil {
clients.IO.PrintWarning(ctx, "%s", style.Sectionf(style.TextSection{
Text: "Could not fetch the remote app manifest",
Secondary: []string{
fetchErr.Error(),
"The template manifest was kept unchanged",
},
}))
Comment on lines +244 to +250

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.

⚠️ question: Could returning the error here bring more confidence for scripts? I'm unsure when this might fail but would hope that invalid app IDs error overall to start

}
}
}

Expand Down Expand Up @@ -289,6 +307,21 @@ func printCreateSuccess(ctx context.Context, clients *shared.ClientFactory, appP
clients.IO.PrintTrace(ctx, slacktrace.CreateSuccess)
}

// fetchAndWriteRemoteManifest fetches the app manifest from remote settings and writes it to the project.
func fetchAndWriteRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token, appID, projectPath string) error {
slackYaml, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID)
if err != nil {
return err
}
data, err := json.MarshalIndent(slackYaml.AppManifest, "", " ")

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: I'm finding some values are escaped but this hasn't caused an error when testing the custom step template:

-           "type": "slack#/types/user_id",
+           "type": "slack#\/types\/user_id",

if err != nil {
return err
}
data = append(data, '\n')
manifestPath := filepath.Join(projectPath, "manifest.json")
return afero.WriteFile(clients.Fs, manifestPath, data, 0644)

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.

🏁 suggestion: We should save the hash alongside this to avoid a confusing prompt of a changed manifest on the first "run" command:

hash, err := clients.Config.ProjectConfig.Cache().NewManifestHash(ctx, upstream.Manifest.AppManifest)
if err != nil {
return types.App{}, "", err
}
if !hash.Equals(saved) {
err := clients.Config.ProjectConfig.Cache().SetManifestHash(ctx, app.AppID, hash)
if err != nil {
return types.App{}, "", err
}
}

$ cd my-test
$ slack run

📚 App Manifest
   Manifest values for this app are overwritten on reinstall

┃ Overwrite manifest on app settings with the project's manifest file?
┃   Yes
┃ ❱ No

}

Comment on lines +310 to +324

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.

📠 suggestion: We might find a new file better for this? It might be shared in other commands or PRs #591!

internal/manifest/export.go

// generateRandomAppName will create a random app name based on two words and a number
func generateRandomAppName() string {
rand.New(rand.NewSource(time.Now().UnixNano()))
Expand Down
100 changes: 96 additions & 4 deletions cmd/project/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"testing"

"github.com/slackapi/slack-cli/internal/api"
"github.com/slackapi/slack-cli/internal/app"
internalApp "github.com/slackapi/slack-cli/internal/app"
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/pkg/create"
Expand All @@ -29,6 +29,7 @@ import (
"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/test/testutil"
"github.com/spf13/afero"
"github.com/spf13/cobra"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
Expand Down Expand Up @@ -983,6 +984,97 @@ func TestCreateCommand_AppFlag(t *testing.T) {
})
}

func TestCreateCommand_AppFlag_FetchesRemoteManifest(t *testing.T) {
var createClientMock *CreateClientMock

mockAuth := types.SlackAuth{
Token: "xoxp-test-token",
TeamDomain: "test-team",
TeamID: "T001",
UserID: "U001",
}
mockManifest := types.SlackYaml{
AppManifest: types.AppManifest{
DisplayInformation: types.DisplayInformation{
Name: "My Remote App",
Description: "An app from remote settings",
},
},
}

setupAppFlagMocks := func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) string {
projectDir := t.TempDir()
createClientMock = new(CreateClientMock)
createClientMock.On("Create", mock.Anything, mock.Anything, mock.Anything).Return(projectDir, nil)
CreateFunc = createClientMock.Create

cm.Os.On("Getwd").Return(projectDir, nil)

err := cm.Fs.MkdirAll(projectDir+"/.slack", 0755)
require.NoError(t, err)
err = afero.WriteFile(cm.Fs, projectDir+"/.slack/hooks.json", []byte("{}"), 0644)
require.NoError(t, err)

cm.IO.On("SelectPrompt", mock.Anything, "Select a category:", mock.Anything, mock.Anything).
Return(iostreams.SelectPromptResponse{Flag: true, Option: "slack-samples/bolt-js-starter-template"}, nil)

cm.Auth.On("Auths", mock.Anything).Return([]types.SlackAuth{mockAuth}, nil)
cm.IO.On("SelectPrompt", mock.Anything, "Select the existing app team", mock.Anything, mock.Anything, mock.Anything).
Return(iostreams.SelectPromptResponse{Prompt: true, Index: 0, Option: mockAuth.TeamDomain}, nil)
cm.IO.On("SelectPrompt", mock.Anything, "Choose the app environment", mock.Anything, mock.Anything, mock.Anything).
Return(iostreams.SelectPromptResponse{Prompt: true, Option: "local"}, nil)

cm.API.On("GetAppStatus", mock.Anything, mockAuth.Token, []string{"A0123456789"}, mockAuth.TeamID).
Return(api.GetAppStatusResult{}, nil)

return projectDir
}

var projectDir string

testutil.TableTestCommand(t, testutil.CommandTests{
"fetches remote manifest after linking app": {
CmdArgs: []string{"my-app", "--template", "slack-samples/bolt-js-starter-template", "--app", "A0123456789", "--environment", "local"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
projectDir = setupAppFlagMocks(t, ctx, cm, cf)

manifestMock := &internalApp.ManifestMockObject{}
manifestMock.On("GetManifestRemote", mock.Anything, mockAuth.Token, "A0123456789").
Return(mockManifest, nil)
cf.AppClient().Manifest = manifestMock
},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
createClientMock.AssertCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything)

manifestData, err := afero.ReadFile(cm.Fs, projectDir+"/manifest.json")
require.NoError(t, err)
assert.Contains(t, string(manifestData), `"name": "My Remote App"`)
assert.Contains(t, string(manifestData), `"description": "An app from remote settings"`)
},
},
"warns on manifest fetch failure": {
CmdArgs: []string{"my-app", "--template", "slack-samples/bolt-js-starter-template", "--app", "A0123456789", "--environment", "local"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
projectDir = setupAppFlagMocks(t, ctx, cm, cf)

manifestMock := &internalApp.ManifestMockObject{}
manifestMock.On("GetManifestRemote", mock.Anything, mockAuth.Token, "A0123456789").
Return(types.SlackYaml{}, slackerror.New("network error"))
cf.AppClient().Manifest = manifestMock
},
ExpectedStdoutOutputs: []string{
"Could not fetch the remote app manifest",
"The template manifest was kept unchanged",
},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
createClientMock.AssertCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything)
},
},
}, func(cf *shared.ClientFactory) *cobra.Command {
return NewCreateCommand(cf)
})
}

var mockCreateLinkAuth = types.SlackAuth{
Token: "xoxp-example",
TeamDomain: "team1",
Expand All @@ -991,8 +1083,6 @@ var mockCreateLinkAuth = types.SlackAuth{
UserID: "U001",
}

// setupCreateLinkMocks prepares the in-memory project config and manifest mocks
// needed by app.LinkExistingApp when called from the create command.
func setupCreateLinkMocks(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
projectDirPath := slackdeps.MockWorkingDirectory
cm.Os.On("Getwd").Return(projectDirPath, nil)
Expand All @@ -1007,8 +1097,10 @@ func setupCreateLinkMocks(t *testing.T, ctx context.Context, cm *shared.ClientsM
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source: %s", err))
}

manifestMock := &app.ManifestMockObject{}
manifestMock := &internalApp.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).
Return(types.SlackYaml{}, nil)
manifestMock.On("GetManifestRemote", mock.Anything, mock.Anything, mock.Anything).
Return(types.SlackYaml{}, nil)
cf.AppClient().Manifest = manifestMock
}
2 changes: 1 addition & 1 deletion cmd/project/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func projectInitCommandRunE(clients *shared.ClientFactory, cmd *cobra.Command, a
_ = create.InstallProjectDependencies(ctx, clients, projectDirPath)

// Add an existing app to the project
err = app.LinkExistingApp(ctx, clients, &types.App{}, true)
_, err = app.LinkExistingApp(ctx, clients, &types.App{}, true)
if err != nil {
// Display the error but continue to init
clients.IO.PrintError(ctx, "%s", err.Error())
Expand Down
Loading