Skip to content

refactor: remove context.Background() references from hooks #50

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mwbrooks merged 3 commits into from
Apr 21, 2025
Merged

refactor: remove context.Background() references from hooks #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8335b77
refactor: remove context.Background() references from hooks
mwbrooks Apr 18, 2025
395ed23
test: fix hooks mocks
mwbrooks Apr 18, 2025
e1b1e44
Merge branch 'main' into mbrooks-hooks-context-background
mwbrooks Apr 21, 2025
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
2 changes: 1 addition & 1 deletion cmd/app/add_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ func prepareAddMocks(t *testing.T, clients *shared.ClientFactory, clientsMock *s
Return("logstash host")

manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{

@mwbrooks mwbrooks Apr 18, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: A common change is updating mocks to support the added ctx context.Context argument.

AppManifest: types.AppManifest{
DisplayInformation: types.DisplayInformation{
Name: team1TeamDomain,
Expand Down
3 changes: 1 addition & 2 deletions cmd/app/uninstall_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ var fakeApp = types.App{
var selectedProdApp = prompts.SelectedApp{Auth: types.SlackAuth{TeamDomain: "team1234"}, App: types.App{AppID: fakeAppID, TeamID: fakeAppTeamID}}

func TestAppsUninstall(t *testing.T) {

testutil.TableTestCommand(t, testutil.CommandTests{
"Successfully uninstall": {
Setup: func(t *testing.T, ctx context.Context, clientsMock *shared.ClientsMock, clients *shared.ClientFactory) {
Expand All @@ -62,7 +61,7 @@ func TestAppsUninstall(t *testing.T) {
clientsMock.ApiInterface.On("UninstallApp", mock.Anything, mock.Anything, fakeAppID, fakeAppTeamID).
Return(nil).Once()
manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).
Return(types.SlackYaml{}, slackerror.New(slackerror.ErrSDKHookNotFound))
clients.AppClient().Manifest = manifestMock
},
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/bulk_delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ func TestBulkDeleteCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/bulk_get_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ func TestBulkGetCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/bulk_put_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ func TestBulkPutCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/count_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ func TestCountCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ func TestDeleteCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/get_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ func TestGetCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/put_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ func TestPutCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ func TestQueryCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/datastore/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ func TestUpdateCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
6 changes: 3 additions & 3 deletions cmd/doctor/check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ func TestDoctorCheckProjectDeps(t *testing.T) {
mockHookUpdate := `{"name": "deno_slack_hooks", "current": "2.6.0", "latest": "2.7.0", "breaking": false, "update": true}`
mockUpdate := fmt.Sprintf(`{"name": "the Slack SDK", "releases": [%s, %s, %s]}`, mockSDKUpdate, mockAPIUpdate, mockHookUpdate)
mockUpdateScript := hooks.HookScript{Command: "echo updates"}
cm.HookExecutor.On("Execute", hooks.HookExecOpts{Hook: mockUpdateScript}).
cm.HookExecutor.On("Execute", mock.Anything, hooks.HookExecOpts{Hook: mockUpdateScript}).
Return(mockUpdate, nil)
return shared.NewClientFactory(cm.MockClientFactory(), func(clients *shared.ClientFactory) {
clients.SDKConfig.WorkingDirectory = "."
Expand Down Expand Up @@ -351,7 +351,7 @@ func TestDoctorCheckProjectTooling(t *testing.T) {
mockHookSetup: func(cm *shared.ClientsMock) *shared.ClientFactory {
mockDoctorHook := `{"versions": [{"name": "deno", "current": "1.0.0"}, {"name": "typescript", "current": "5.4.3"}]}`
mockDoctorScript := hooks.HookScript{Command: "echo checkup"}
cm.HookExecutor.On("Execute", hooks.HookExecOpts{Hook: mockDoctorScript}).
cm.HookExecutor.On("Execute", mock.Anything, hooks.HookExecOpts{Hook: mockDoctorScript}).
Return(mockDoctorHook, nil)
return shared.NewClientFactory(cm.MockClientFactory(), func(clients *shared.ClientFactory) {
clients.SDKConfig.WorkingDirectory = "."
Expand Down Expand Up @@ -379,7 +379,7 @@ func TestDoctorCheckProjectTooling(t *testing.T) {
mockHookSetup: func(cm *shared.ClientsMock) *shared.ClientFactory {
mockDoctorHook := `{"versions": [{"name": "deno", "current": "1.0.0", "message": "Secure runtimes make safer code", "error": {"message": "Something isn't right with this installation"}}]}`
mockDoctorScript := hooks.HookScript{Command: "echo checkup"}
cm.HookExecutor.On("Execute", hooks.HookExecOpts{Hook: mockDoctorScript}).
cm.HookExecutor.On("Execute", mock.Anything, hooks.HookExecOpts{Hook: mockDoctorScript}).

@mwbrooks mwbrooks Apr 18, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: I know a few of these lines are getting longer and could be expanded to multiple lines, but I wanted to keep the changes low since this PR was already getting a little large.

Return(mockDoctorHook, nil)
return shared.NewClientFactory(cm.MockClientFactory(), func(clients *shared.ClientFactory) {
clients.SDKConfig.WorkingDirectory = "."
Expand Down
2 changes: 1 addition & 1 deletion cmd/doctor/doctor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func doctorHook(ctx context.Context, clients *shared.ClientFactory) (DoctorHookJ
var hookExecOpts = hooks.HookExecOpts{
Hook: clients.SDKConfig.Hooks.Doctor,
}
getDoctorHookJSON, err := clients.HookExecutor.Execute(hookExecOpts)
getDoctorHookJSON, err := clients.HookExecutor.Execute(ctx, hookExecOpts)

@mwbrooks mwbrooks Apr 18, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: This is the final result of the work - the new signature is .Execute(ctx, opts).

if err != nil {
return DoctorHookJSON{}, err
}
Expand Down
8 changes: 4 additions & 4 deletions cmd/doctor/doctor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,11 @@ func TestDoctorCommand(t *testing.T) {
clientsMock.Config.SystemConfig = scm
mockDoctorScript := hooks.HookScript{Command: "echo checkup"}
mockDoctorHook := `{"versions": [{"name": "node", "current": "20.11.1"}]}`
clientsMock.HookExecutor.On("Execute", hooks.HookExecOpts{Hook: mockDoctorScript}).
clientsMock.HookExecutor.On("Execute", mock.Anything, hooks.HookExecOpts{Hook: mockDoctorScript}).
Return(mockDoctorHook, nil)
mockUpdateScript := hooks.HookScript{Command: "echo update"}
mockUpdateHook := `{"name": "the Slack SDK", "releases": [{"name": "@slack/bolt", "current": "1.0.0", "latest": "2.2.2", "breaking": true, "update": true}]}`
clientsMock.HookExecutor.On("Execute", hooks.HookExecOpts{Hook: mockUpdateScript}).
clientsMock.HookExecutor.On("Execute", mock.Anything, hooks.HookExecOpts{Hook: mockUpdateScript}).
Return(mockUpdateHook, nil)
clients := shared.NewClientFactory(clientsMock.MockClientFactory(), func(clients *shared.ClientFactory) {
clients.SDKConfig.WorkingDirectory = "."
Expand Down Expand Up @@ -357,7 +357,7 @@ func TestDoctorHook(t *testing.T) {
mockHookSetup: func(cm *shared.ClientsMock) *shared.ClientFactory {
mockDoctorHook := `{"versions": [{"name": "deno", "current": "1.0.0"}, {"name": "typescript", "current": "5.4.3"}]}`
mockDoctorScript := hooks.HookScript{Command: "echo checkup"}
cm.HookExecutor.On("Execute", hooks.HookExecOpts{Hook: mockDoctorScript}).
cm.HookExecutor.On("Execute", mock.Anything, hooks.HookExecOpts{Hook: mockDoctorScript}).
Return(mockDoctorHook, nil)
return shared.NewClientFactory(cm.MockClientFactory(), func(clients *shared.ClientFactory) {
clients.SDKConfig.WorkingDirectory = "."
Expand Down Expand Up @@ -390,7 +390,7 @@ func TestDoctorHook(t *testing.T) {
mockHookSetup: func(cm *shared.ClientsMock) *shared.ClientFactory {
mockDoctorHook := `{"versions": [{"name": "deno", "current": "1.0.0", "message": "Secure runtimes make safer code", "error": {"message": "Something isn't right with this installation"}}]}`
mockDoctorScript := hooks.HookScript{Command: "echo checkup"}
cm.HookExecutor.On("Execute", hooks.HookExecOpts{Hook: mockDoctorScript}).
cm.HookExecutor.On("Execute", mock.Anything, hooks.HookExecOpts{Hook: mockDoctorScript}).
Return(mockDoctorHook, nil)
return shared.NewClientFactory(cm.MockClientFactory(), func(clients *shared.ClientFactory) {
clients.SDKConfig.WorkingDirectory = "."
Expand Down
1 change: 1 addition & 0 deletions cmd/env/add_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ func Test_Env_AddCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/env/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ func Test_Env_ListCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/env/remove_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ func Test_Env_RemoveCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/externalauth/add_secret_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ func TestExternalAuthAddClientSecretCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/externalauth/add_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ func TestExternalAuthAddCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/externalauth/remove_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ func TestExternalAuthRemoveCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
1 change: 1 addition & 0 deletions cmd/externalauth/select_auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ func TestExternalAuthSelectAuthCommandPreRun(t *testing.T) {
"GetManifestLocal",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(
tt.mockManifestResponse,
tt.mockManifestError,
Expand Down
5 changes: 3 additions & 2 deletions cmd/manifest/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func getManifestInfo(ctx context.Context, clients *shared.ClientFactory, cmd *co
}
switch {
case source.Equals(config.MANIFEST_SOURCE_LOCAL):
return getManifestInfoProject(clients)
return getManifestInfoProject(ctx, clients)
case source.Equals(config.MANIFEST_SOURCE_REMOTE):
return getManifestInfoRemote(ctx, clients)
Comment on lines +109 to 111

@mwbrooks mwbrooks Apr 18, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: It's nice to see that both getManifestInfoProject(ctx, clients) and getManifestInfoRemote(ctx, clients) have the same function signatures.

default:
Expand All @@ -115,8 +115,9 @@ func getManifestInfo(ctx context.Context, clients *shared.ClientFactory, cmd *co
}

// getManifestInfoProject gathers app manifest information from "get-manifest"
func getManifestInfoProject(clients *shared.ClientFactory) (types.AppManifest, error) {
func getManifestInfoProject(ctx context.Context, clients *shared.ClientFactory) (types.AppManifest, error) {
slackManifest, err := clients.AppClient().Manifest.GetManifestLocal(
ctx,
clients.SDKConfig,
clients.HookExecutor,
)
Expand Down
6 changes: 3 additions & 3 deletions cmd/manifest/info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func TestInfoCommand(t *testing.T) {
CmdArgs: []string{"--source", "paper"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
cf.SDKConfig = hooks.NewSDKConfigMock()
cm.HookExecutor.On("Execute", mock.Anything).Return("", nil)
cm.HookExecutor.On("Execute", mock.Anything, mock.Anything).Return("", nil)
},
ExpectedError: slackerror.New(slackerror.ErrInvalidFlag).
WithMessage("The \"--source\" flag must be \"local\" or \"remote\""),
Expand All @@ -61,7 +61,7 @@ func TestInfoCommand(t *testing.T) {
CmdArgs: []string{"--source", "local"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{
AppManifest: types.AppManifest{
DisplayInformation: types.DisplayInformation{
Name: "app001",
Expand Down Expand Up @@ -125,7 +125,7 @@ func TestInfoCommand(t *testing.T) {
cm.Os.AddDefaultMocks()
cf.SDKConfig.WorkingDirectory = "."
manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{
AppManifest: types.AppManifest{
DisplayInformation: types.DisplayInformation{
Name: "app002",
Expand Down
4 changes: 2 additions & 2 deletions cmd/platform/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ func hasValidDeploymentMethod(
}
switch {
case manifestSource.Equals(config.MANIFEST_SOURCE_LOCAL):
manifest, err = clients.AppClient().Manifest.GetManifestLocal(clients.SDKConfig, clients.HookExecutor)
manifest, err = clients.AppClient().Manifest.GetManifestLocal(ctx, clients.SDKConfig, clients.HookExecutor)
if err != nil {
return err
}
Expand Down Expand Up @@ -212,7 +212,7 @@ func deployHook(ctx context.Context, clients *shared.ClientFactory) (*logger.Log
shell := hooks.HookExecutorDefaultProtocol{
IO: clients.IO,
}
if _, err := shell.Execute(hookExecOpts); err != nil {
if _, err := shell.Execute(ctx, hookExecOpts); err != nil {

@zimeg zimeg Apr 21, 2025

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.

🔭 This change is super great to find!

return &log, err
}
// Follow successful hook executions with a newline to match section formatting
Expand Down
4 changes: 2 additions & 2 deletions cmd/platform/deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func TestDeployCommand(t *testing.T) {
teamAppSelectPromptFunc = appSelectMock.TeamAppSelectPrompt

manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{
AppManifest: types.AppManifest{
Settings: &types.AppSettings{
FunctionRuntime: types.SLACK_HOSTED,
Expand Down Expand Up @@ -150,7 +150,7 @@ func TestDeployCommand_HasValidDeploymentMethod(t *testing.T) {
clientsMock := shared.NewClientsMock()
clients := shared.NewClientFactory(clientsMock.MockClientFactory(), func(clients *shared.ClientFactory) {
manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(tt.manifest, tt.manifestError)
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(tt.manifest, tt.manifestError)
clients.AppClient().Manifest = manifestMock
projectConfigMock := config.NewProjectConfigMock()
projectConfigMock.On("GetManifestSource", mock.Anything).
Expand Down
9 changes: 5 additions & 4 deletions cmd/triggers/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command) error {

var triggerArg api.TriggerRequest
if createFlags.triggerDef != "" {
triggerArg, err = triggerRequestFromDef(clients, createFlags, app.IsDev)
triggerArg, err = triggerRequestFromDef(ctx, clients, createFlags, app.IsDev)
if err != nil {
return err
}
Expand Down Expand Up @@ -305,7 +305,7 @@ func triggerRequestFromFlags(flags createCmdFlags, isDev bool) api.TriggerReques
return req
}

func triggerRequestViaHook(clients *shared.ClientFactory, path string, isDev bool) (api.TriggerRequest, error) {
func triggerRequestViaHook(ctx context.Context, clients *shared.ClientFactory, path string, isDev bool) (api.TriggerRequest, error) {

@mwbrooks mwbrooks Apr 18, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: Example of the cascading changes to pass ctx into areas where .Execute is called.

@zimeg zimeg Apr 21, 2025

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.

@mwbrooks Legend ⭐

These changes seem tedious, but I am so hopeful that future changes can make use of the real ctx without hassle now.

if !clients.SDKConfig.Hooks.GetTrigger.IsAvailable() {
return api.TriggerRequest{}, slackerror.New(slackerror.ErrSDKHookGetTriggerNotFound)
}
Expand All @@ -319,6 +319,7 @@ func triggerRequestViaHook(clients *shared.ClientFactory, path string, isDev boo
hookExecOpts.Env[name] = val
}
triggerDefAsStr, err := clients.HookExecutor.Execute(
ctx,
hookExecOpts,
)
if err != nil {
Expand Down Expand Up @@ -350,10 +351,10 @@ func triggerRequestFromJSONFile(clients *shared.ClientFactory, path string, isDe
return req, nil
}

func triggerRequestFromDef(clients *shared.ClientFactory, flags createCmdFlags, isDev bool) (api.TriggerRequest, error) {
func triggerRequestFromDef(ctx context.Context, clients *shared.ClientFactory, flags createCmdFlags, isDev bool) (api.TriggerRequest, error) {
if strings.HasSuffix(flags.triggerDef, ".json") {
return triggerRequestFromJSONFile(clients, flags.triggerDef, isDev)
}

return triggerRequestViaHook(clients, flags.triggerDef, isDev)
return triggerRequestViaHook(ctx, clients, flags.triggerDef, isDev)
}
6 changes: 3 additions & 3 deletions cmd/triggers/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ func TestTriggersCreateCommand(t *testing.T) {
appSelectTeardown = setupMockCreateAppSelection(installedProdApp)
// TODO: testing chicken and egg: we need the default mocks in place before we can use any of the `clients` methods

@zimeg zimeg Apr 21, 2025

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.

📚 Unrelated to the changes of this PR, we might revisit these comments soon since setting up default mocks is seeming standard before customized mocks from what I can tell.

@mwbrooks mwbrooks Apr 21, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🗒️ Good call! I'll add a note to loop back on this after I finish the context work. It almost looks like we can just remove this TODO because it's setting up the default mocks and then adding custom ones. 🤔

clientsMock.AddDefaultMocks()
clientsMock.HookExecutor.On("Execute", mock.Anything).Return("", nil)
clientsMock.HookExecutor.On("Execute", mock.Anything, mock.Anything).Return("", nil)
err := clients.AppClient().SaveDeployed(ctx, fakeApp)
require.NoError(t, err, "Cant write apps.json")
},
Expand All @@ -275,7 +275,7 @@ func TestTriggersCreateCommand(t *testing.T) {
Return(types.EVERYONE, []string{}, nil).Once()
// TODO: testing chicken and egg: we need the default mocks in place before we can use any of the `clients` methods
clientsMock.AddDefaultMocks()
clientsMock.HookExecutor.On("Execute", mock.Anything).Return(`{}`, nil)
clientsMock.HookExecutor.On("Execute", mock.Anything, mock.Anything).Return(`{}`, nil)
err := clients.AppClient().SaveDeployed(ctx, fakeApp)
require.NoError(t, err, "Cant write apps.json")
var content = `export default {}`
Expand All @@ -287,7 +287,7 @@ func TestTriggersCreateCommand(t *testing.T) {
appSelectTeardown()
},
ExpectedAsserts: func(t *testing.T, ctx context.Context, clientsMock *shared.ClientsMock) {
clientsMock.HookExecutor.AssertCalled(t, "Execute", mock.Anything)
clientsMock.HookExecutor.AssertCalled(t, "Execute", mock.Anything, mock.Anything)
clientsMock.ApiInterface.AssertCalled(t, "WorkflowsTriggersCreate", mock.Anything, mock.Anything, mock.Anything)
},
},
Expand Down
2 changes: 1 addition & 1 deletion cmd/triggers/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func TriggerGenerate(ctx context.Context, clients *shared.ClientFactory, app typ
selectedTriggerDef = selection.Option
}

triggerArg, err := triggerRequestFromDef(clients, createCmdFlags{triggerDef: selectedTriggerDef}, app.IsDev)
triggerArg, err := triggerRequestFromDef(ctx, clients, createCmdFlags{triggerDef: selectedTriggerDef}, app.IsDev)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/triggers/generate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ func prepareMocks(t *testing.T, triggersListResponse []types.DeployedTrigger, gl
clientsMock.ApiInterface.On("WorkflowsTriggersList", mock.Anything, mock.Anything, mock.Anything).Return(triggersListResponse, "", nil)
clientsMock.Os.On("Glob", mock.Anything).Return(globResponse, nil)
clientsMock.ApiInterface.On("WorkflowsTriggersCreate", mock.Anything, mock.Anything, mock.Anything).Return(triggersCreateResponse, triggersCreateResponseError)
clientsMock.HookExecutor.On("Execute", mock.Anything).Return(`{}`, nil)
clientsMock.HookExecutor.On("Execute", mock.Anything, mock.Anything).Return(`{}`, nil)

clientsMock.AddDefaultMocks()

Expand Down
2 changes: 1 addition & 1 deletion cmd/triggers/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func runUpdateCommand(clients *shared.ClientFactory, cmd *cobra.Command) error {

var triggerArg api.TriggerRequest
if updateFlags.triggerDef != "" {
triggerArg, err = triggerRequestFromDef(clients, updateFlags.createCmdFlags, app.IsDev)
triggerArg, err = triggerRequestFromDef(ctx, clients, updateFlags.createCmdFlags, app.IsDev)
if err != nil {
return err
}
Expand Down
Loading