refactor: remove context.Background() references from hooks by mwbrooks · Pull Request #50 · slackapi/slack-cli

mwbrooks · 2025-04-18T03:09:20Z

Summary

~~Draft until after our upcoming release.~~ Release v3.0.5 is ready

This pull request updates the package hooks by removing all references to context.Background().

Reviewers

This is a bit of a nasty change, so I've dedicated a PR to it.

The key change updates .Execute(opts) to .Execute(ctx, opts). This causes a ripple effect of changes in order to pass ctx into the hooks executor. The majority of the changes occur in the tests, where mocks must be updated to work with the new argument. 🪝

Really appreciate the time of reviewer going through these changes!

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2025-04-18T03:11:13Z

Codecov Report

Attention: Patch coverage is 95.45455% with 2 lines in your changes missing coverage. Please review.

Project coverage is 62.89%. Comparing base (f3ba824) to head (e1b1e44).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/platform/deploy.go	0.00%	1 Missing ⚠️
internal/update/sdk.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
- Coverage   62.91%   62.89%   -0.02%     
==========================================
  Files         210      210              
  Lines       22152    22154       +2     
==========================================
- Hits        13936    13934       -2     
- Misses       7132     7133       +1     
- Partials     1084     1087       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mwbrooks

📝 Notes to help the very much appreciated reviewer!

mwbrooks · 2025-04-18T03:10:14Z


 	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{


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

mwbrooks · 2025-04-18T03:11:42Z

 				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}).


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.

mwbrooks · 2025-04-18T03:12:30Z

 		Hook: clients.SDKConfig.Hooks.Doctor,
 	}
-	getDoctorHookJSON, err := clients.HookExecutor.Execute(hookExecOpts)
+	getDoctorHookJSON, err := clients.HookExecutor.Execute(ctx, hookExecOpts)


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

mwbrooks · 2025-04-18T03:14:06Z

+		return getManifestInfoProject(ctx, clients)
 	case source.Equals(config.MANIFEST_SOURCE_REMOTE):
 		return getManifestInfoRemote(ctx, clients)


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

mwbrooks · 2025-04-18T03:15:39Z

 }

-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) {


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

@mwbrooks Legend ⭐

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

mwbrooks · 2025-04-18T03:17:05Z

+	GetManifestLocal(ctx context.Context, sdkConfig hooks.SDKCLIConfig, hookExecutor hooks.HookExecutor) (types.SlackYaml, error)
 	GetManifestRemote(ctx context.Context, token string, appID string) (types.SlackYaml, error)


note: It's nice to see that both GetManifestLocal(ctx, ... and GetManifestRemote(ctx, ... now except context.

mwbrooks · 2025-04-18T03:18:54Z


 // Execute processes the data received by the SDK.
-func (e *HookExecutorDefaultProtocol) Execute(opts HookExecOpts) (string, error) {
+func (e *HookExecutorDefaultProtocol) Execute(ctx context.Context, opts HookExecOpts) (string, error) {


⭐ ⭐ ⭐
note: This is 1 of 2 key changes in this PR. Here we update the default hook executor protocol to accept a context argument. This allows us to remove the context.Background() usage inside this function.
⭐ ⭐ ⭐

@mwbrooks Fantastic catch here!

IIRC a "background" context was used as a quick workaround for debug printing. It's now more clear that a follow up for this is important and having it broken into a standalone PR makes reviewing swell 🫡

mwbrooks · 2025-04-18T03:19:25Z


 // Execute processes the data received by the SDK.
-func (e *HookExecutorMessageBoundaryProtocol) Execute(opts HookExecOpts) (string, error) {
+func (e *HookExecutorMessageBoundaryProtocol) Execute(ctx context.Context, opts HookExecOpts) (string, error) {


⭐ ⭐ ⭐
note: This is 2 of 2 key changes in this PR. Here we update the V2 hook executor protocol to accept a context argument. This allows us to remove the context.Background() usage inside this function.
⭐ ⭐ ⭐

mwbrooks · 2025-04-18T03:21:07Z

 		AuthTokens: authTokens,
 	}
-	if err := clients.Runtime.PreparePackage(clients.SDKConfig, clients.HookExecutor, preparePackageOpts); err != nil {
+	if err := clients.Runtime.PreparePackage(ctx, clients.SDKConfig, clients.HookExecutor, preparePackageOpts); err != nil {


note: We also needed to update the package runtime interface for PreparePackage to accept a ctx argument. On the upside, this aligns with the other functions in the interface, which also accept a ctx.

mwbrooks · 2025-04-18T03:22:38Z

 	SetVersion(string)
 	HooksJSONTemplate() []byte
-	PreparePackage(hooks.SDKCLIConfig, hooks.HookExecutor, types.PreparePackageOpts) error
+	PreparePackage(context.Context, hooks.SDKCLIConfig, hooks.HookExecutor, types.PreparePackageOpts) error


note: Here we are updating the package runtime interface. Now both InstallProjectDependencies(ctx, ... and PreparePackage(ctx, ... accept context.

zimeg

@mwbrooks All super awesome changes within! 👾 ✨

The key change updates .Execute(opts) to .Execute(ctx, opts).

This is big 🔑 I know a missing context here has confused me before!

Finding that we were using "background" contexts and replacing it with the shared and single context used elsewhere is so good for keeping things consistent.

With this in mind, all of the other (many) changes go well!

I might've commented this elsewhere, but thanks for keeping these changes scoped to single PRs 💌

zimeg · 2025-04-21T19:19:24Z

 		IO: clients.IO,
 	}
-	if _, err := shell.Execute(hookExecOpts); err != nil {
+	if _, err := shell.Execute(ctx, hookExecOpts); err != nil {


🔭 This change is super great to find!

zimeg · 2025-04-21T19:20:27Z

 }

-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 Legend ⭐

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

zimeg · 2025-04-21T19:22:08Z

@@ -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


📚 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.

🗒️ 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. 🤔

zimeg · 2025-04-21T19:23:46Z

 	}

-	slackManifestInfo, err := hookExecutor.Execute(manifestHookOpts)
+	slackManifestInfo, err := hookExecutor.Execute(ctx, manifestHookOpts)


One 👏 true 👏 context 👏 ✨

zimeg · 2025-04-21T19:25:49Z


 // Execute processes the data received by the SDK.
-func (e *HookExecutorDefaultProtocol) Execute(opts HookExecOpts) (string, error) {
+func (e *HookExecutorDefaultProtocol) Execute(ctx context.Context, opts HookExecOpts) (string, error) {


@mwbrooks Fantastic catch here!

IIRC a "background" context was used as a quick workaround for debug printing. It's now more clear that a follow up for this is important and having it broken into a standalone PR makes reviewing swell 🫡

zimeg · 2025-04-21T19:28:05Z

+			mockHookExecutor.On("Execute", mock.Anything, mock.Anything).
 				Run(func(args mock.Arguments) {
-					opts := args.Get(0).(hooks.HookExecOpts)
+					opts := args.Get(1).(hooks.HookExecOpts)
 					_, err := opts.Stdout.Write([]byte(tt.hookExecuteStdout))
 					require.NoError(t, err)
 				}).


👁️‍🗨️ Good catch!!

mwbrooks · 2025-04-21T20:14:32Z

🙏🏻 @zimeg Thanks for the quick review and I've made a few notes about your suggestions for the future. Always appreciate that you're willing to review these PRs - they're not exciting, but hopefully moving the code into a better place. One PR at a time.

mwbrooks added 2 commits April 17, 2025 18:58

refactor: remove context.Background() references from hooks

8335b77

test: fix hooks mocks

395ed23

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Apr 18, 2025

mwbrooks added this to the Next Release milestone Apr 18, 2025

mwbrooks self-assigned this Apr 18, 2025

mwbrooks commented Apr 18, 2025

View reviewed changes

zimeg modified the milestones: v3.0.5, Next Release Apr 18, 2025

Merge branch 'main' into mbrooks-hooks-context-background

e1b1e44

mwbrooks marked this pull request as ready for review April 21, 2025 18:57

mwbrooks requested a review from a team as a code owner April 21, 2025 18:57

zimeg approved these changes Apr 21, 2025

View reviewed changes

mwbrooks merged commit edb8e97 into main Apr 21, 2025

mwbrooks deleted the mbrooks-hooks-context-background branch April 21, 2025 20:14

		GetManifestLocal(ctx context.Context, sdkConfig hooks.SDKCLIConfig, hookExecutor hooks.HookExecutor) (types.SlackYaml, error)
		GetManifestRemote(ctx context.Context, token string, appID string) (types.SlackYaml, error)

		@@ -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

Uh oh!

Conversation

mwbrooks commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Reviewers

Requirements

Uh oh!

codecov Bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Apr 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwbrooks commented Apr 18, 2025 •

edited

Loading

codecov Bot commented Apr 18, 2025 •

edited

Loading