refactor: update .Context() references for consistency #33

mwbrooks · 2025-04-11T18:18:28Z

Summary

🎶 We all live in a yellow context, a yellow context, a yellow context 🛥️

This pull request combs through our code to update references to .Context() for more consistency. A few common changes are:

Pulling out context from someFunc(cmd.Context(), ...) to someFunc(ctx, ...)
- Verifying that cmd.Context() is our mocked context, which can happen from:
  - cmd.SetContext(ctxMock) before Cobra execution
  - cmd.ExecuteContext(ctxMock) after Cobra execution
Catching a few missed places where .Setup provides the mocked context

Reviewers

Production code is modified in this PR, but I don't think we'll see any regressions or issues.

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.

…into mbrooks-slackcontext-tabletestcommand-expectedasserts-background

…edasserts-background-context

codecov · 2025-04-11T18:29:07Z

Codecov Report

Attention: Patch coverage is 59.72222% with 29 lines in your changes missing coverage. Please review.

Project coverage is 62.92%. Comparing base (c084fde) to head (94a3fdf).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/triggers/access.go	16.66%	5 Missing ⚠️
cmd/auth/list.go	50.00%	4 Missing ⚠️
cmd/triggers/create.go	0.00%	4 Missing ⚠️
cmd/root.go	57.14%	3 Missing ⚠️
internal/update/update.go	0.00%	3 Missing ⚠️
cmd/project/create.go	33.33%	2 Missing ⚠️
cmd/upgrade/upgrade.go	0.00%	2 Missing ⚠️
internal/update/cli.go	0.00%	2 Missing ⚠️
cmd/app/app.go	50.00%	1 Missing ⚠️
cmd/platform/run.go	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
- Coverage   62.95%   62.92%   -0.04%     
==========================================
  Files         210      210              
  Lines       22127    22149      +22     
==========================================
+ Hits        13930    13937       +7     
- Misses       7113     7127      +14     
- Partials     1084     1085       +1

☔ 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

🦮 Comments to guide the reviewer safely toward the end!

mwbrooks · 2025-04-11T18:30:46Z

cmd/app/app.go

+			ctx := cmd.Context()
 			if cmd.CalledAs() == "workspace" {
-				clients.IO.PrintInfo(cmd.Context(), false, fmt.Sprintf(
+				clients.IO.PrintInfo(ctx, false, fmt.Sprintf(


note: I think this is a better pattern and I remember @zimeg has also asked that we reduce the amount of arguments that are functions calls. The main benefit is that other functions in the code block can use the same context ctx ensuring more consistency through the code.

I noticed too you're updating it to be a first declaration in a function, which I'm so much a fan of too 🙏 ✨

In later comments you note that we often avoid modifying ctx as well and I think this makes it more clear that it's meant to be more of a function argument and not a variable to change 😉

mwbrooks · 2025-04-11T18:32:47Z

cmd/app/link_test.go

 				}, nil)
 				cm.AddDefaultMocks()
-				setupAppLinkCommandMocks(t, cm, cf)
+				setupAppLinkCommandMocks(t, ctx, cm, cf)


note: This catches a .Setup(t *testing.T, ctx context.Context, ...) that provides the ctx. We can now use it instead of setupAppLinkCommandMocks(...) creating a non-mocked context.

mwbrooks · 2025-04-11T18:37:37Z

cmd/collaborators/update.go

-	var span opentracing.Span
 	ctx := cmd.Context()
-	span, ctx = opentracing.StartSpanFromContext(ctx, "cmd.Collaborators.Update")
+	span, _ := opentracing.StartSpanFromContext(ctx, "cmd.Collaborators.Update")


thought: This is up for debate. 🧠 OpenTracing span's return a new context that contains the span (and all of our values). Right now, we inconsistently replace the current ctx with the span's ctx. I've leaned toward not replacing our ctx because:

We never access the span through the ctx AFAIK

Instead, we often call span.Context()... to get it and modify it for API calls

We are moving toward a rule of thumb where the context should not be modified after a Cobra Execution

@mwbrooks Thanks for calling this out. How we use span in context is interesting to me.

We might consider follow ups for this, but IIRC spans are most useful for collecting traces that measure the beginning and end of function calls? I'm not sure if these can be gathered now, but we might find tools to visualize these useful in debugging slow processes 🔍

Not updating this ctx might cause strangeness in the trace callstack and make for confusing debugs if we explore this, but I have not tried it at all!

No blocker for this PR since we might consider this a standalone task altogether, but I'm also curious if this isn't seeming like the right use of context to you 🤔

Making this more consistent overall is a nice change in this PR too!

Yea, you bring up a good point about the span's forming a track callback. I'd expect that we'd want to update the context in that case.

Since we don't lean too heavily on the traces, I'll keep this PR as-is. The intention is to make everything consistent (not updating the context). Then, like you said, we can have a follow-up PR that updates everything together.

mwbrooks · 2025-04-11T19:23:02Z

cmd/root.go

 	cobra.OnInitialize(func() {
-		err := InitConfig(clients, rootCmd)
+		ctx := rootCmd.Context()
+		err := InitConfig(ctx, clients, rootCmd)
 		if err != nil {
-			clients.IO.PrintError(rootCmd.Context(), err.Error())
+			clients.IO.PrintError(ctx, err.Error())
 			clients.Os.Exit(int(iostreams.ExitError))
 		}
 	})
 	// Since we use the *E cobra lifecycle methods, OnFinalize is one of the few ways we can ensure something _always_ runs at the end of any command invocation, regardless if an error is raised or not during execution.
 	cobra.OnFinalize(func() {
-		cleanup(rootCmd.Context(), clients)
+		ctx := rootCmd.Context()
+		cleanup(ctx, clients)
 	})


note: This is a unique area of our code w.r.t context.

The cobra.OnInitialize(...) and cobra.OnFinalize(...) are lifecycle functions called after rootCmd.ExecuteContext(...). I think it would be a nice addition for Cobra to pass the cmd cobra.Command into these functions, but it doesn't.

Since the context may change dramatically before-execution and after-execution, we can get the most up-to-date context with rootCmd.Context(). I think...I haven't tested what happens when you set sub-command sets the context, whether it rolls up to the root command.

🤔 This is so interesting too!

I'm still thinking about the span example and updating context throughout the function callstack and it might make sense that values set in "lower" levels of context aren't shared "above"? At least if the ctx is not returned...

context package that makes it easy to pass request-scoped values

Function calls might be similar to "requests" in this context 🔗 https://go.dev/blog/context

I agree that this is a confusing area overall though.

It's not clear to me how immutable values and pointers pair sometimes... But I am finding that we're setting the context as expected and I'm not finding that cobra modifies this:

slack-cli/cmd/root.go

Line 371 in b76e933

if err := rootCmd.ExecuteContext(ctx); err != nil {

To me this seems like a fine change, but I'm also curious if we can pass ctx to Init instead?

FWIW the current implementation seems to work when gathering context values from within a command RunE handler 👾

Good points! Given how we use ctx in the OnInitialize and OnFinalize, I think we could safely use the ctx passed to Init. The upside is that the code is cleaner and easier to read.

Happy to make that change now before merging this PR!

Commit 9f25031 passes ctx into Init(ctx) and uses it for OnInitialize and OnFinalize. All the magic is removed! 🪄 🙅🏻

zimeg

@mwbrooks LGTM and swimmingly so 🫧 🐟

This PR has super good improvements to code health and asks a few great questions. Thanks a ton for making comments about these.

I responded with a few ideas about how we might want to use ctx throughout command lifecycles and with spans but these might both be explorations for follow up PRs. But no blocking notes within 🎶 🚢 💨

zimeg · 2025-04-11T20:29:01Z

cmd/collaborators/update.go

-	var span opentracing.Span
 	ctx := cmd.Context()
-	span, ctx = opentracing.StartSpanFromContext(ctx, "cmd.Collaborators.Update")
+	span, _ := opentracing.StartSpanFromContext(ctx, "cmd.Collaborators.Update")


@mwbrooks Thanks for calling this out. How we use span in context is interesting to me.

We might consider follow ups for this, but IIRC spans are most useful for collecting traces that measure the beginning and end of function calls? I'm not sure if these can be gathered now, but we might find tools to visualize these useful in debugging slow processes 🔍

Not updating this ctx might cause strangeness in the trace callstack and make for confusing debugs if we explore this, but I have not tried it at all!

No blocker for this PR since we might consider this a standalone task altogether, but I'm also curious if this isn't seeming like the right use of context to you 🤔

zimeg · 2025-04-11T20:30:48Z

cmd/collaborators/update.go

-	var span opentracing.Span
 	ctx := cmd.Context()
-	span, ctx = opentracing.StartSpanFromContext(ctx, "cmd.Collaborators.Update")
+	span, _ := opentracing.StartSpanFromContext(ctx, "cmd.Collaborators.Update")


Making this more consistent overall is a nice change in this PR too!

zimeg · 2025-04-11T20:32:55Z

cmd/app/app.go

+			ctx := cmd.Context()
 			if cmd.CalledAs() == "workspace" {
-				clients.IO.PrintInfo(cmd.Context(), false, fmt.Sprintf(
+				clients.IO.PrintInfo(ctx, false, fmt.Sprintf(


I noticed too you're updating it to be a first declaration in a function, which I'm so much a fan of too 🙏 ✨

In later comments you note that we often avoid modifying ctx as well and I think this makes it more clear that it's meant to be more of a function argument and not a variable to change 😉

zimeg · 2025-04-11T21:11:53Z

cmd/root.go

 	cobra.OnInitialize(func() {
-		err := InitConfig(clients, rootCmd)
+		ctx := rootCmd.Context()
+		err := InitConfig(ctx, clients, rootCmd)
 		if err != nil {
-			clients.IO.PrintError(rootCmd.Context(), err.Error())
+			clients.IO.PrintError(ctx, err.Error())
 			clients.Os.Exit(int(iostreams.ExitError))
 		}
 	})
 	// Since we use the *E cobra lifecycle methods, OnFinalize is one of the few ways we can ensure something _always_ runs at the end of any command invocation, regardless if an error is raised or not during execution.
 	cobra.OnFinalize(func() {
-		cleanup(rootCmd.Context(), clients)
+		ctx := rootCmd.Context()
+		cleanup(ctx, clients)
 	})


🤔 This is so interesting too!

I'm still thinking about the span example and updating context throughout the function callstack and it might make sense that values set in "lower" levels of context aren't shared "above"? At least if the ctx is not returned...

context package that makes it easy to pass request-scoped values

Function calls might be similar to "requests" in this context 🔗 https://go.dev/blog/context

I agree that this is a confusing area overall though.

It's not clear to me how immutable values and pointers pair sometimes... But I am finding that we're setting the context as expected and I'm not finding that cobra modifies this:

slack-cli/cmd/root.go

Line 371 in b76e933

if err := rootCmd.ExecuteContext(ctx); err != nil {

To me this seems like a fine change, but I'm also curious if we can pass ctx to Init instead?

FWIW the current implementation seems to work when gathering context values from within a command RunE handler 👾

zimeg · 2025-04-11T21:20:47Z

internal/config/project_test.go

 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			ctx := t.Context()
+			ctx := slackcontext.MockContext(t.Context())


👁️‍🗨️

zimeg · 2025-04-11T21:20:53Z

internal/slacktrace/slacktrace.go

 //
 // use,
-// clients.IO.PrintTrace(cmd.Context(), slacktrace.<NAMEDCONST>)
+// clients.IO.PrintTrace(ctx, slacktrace.<NAMEDCONST>)


😌 This is so useful for encouraging shared styles

…edasserts-background-context

mwbrooks · 2025-04-14T20:35:14Z

Thanks for the thorough review @zimeg and suggesting the change to Init(ctx) 🙇🏻 Merging!

mwbrooks added 12 commits April 2, 2025 12:44

refactor: Add slackcontext package to DRY context management

4add4cc

refactor: Revert changes to unrelated error codes

6c8907a

test: mock context in TableTestCommand

f7b9014

test: mock context in TableTestCommand ExpectedAsserts

b8f68e9

test: use ctx in mocked ExpectedAsserts

9c816be

test: use ctx in mocked ExpectedAsserts to link_test.go

5f69d23

test: use ctx in mocked ExpectedAsserts to update_test.go

c4b7697

test: use .MockContext(...) instead of context.Background()

63a99c8

test: update context references for .Setup

ad91292

Merge branch 'mbrooks-slackcontext-tabletestcommand-expectedasserts' …

158b3f6

…into mbrooks-slackcontext-tabletestcommand-expectedasserts-background

refactor: update .Context() references for consistency

47a5bcd

refactor: update span to not return a ctx since it's locally scoped

ca08f10

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 11, 2025

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

mwbrooks self-assigned this Apr 11, 2025

Merge branch 'main' into mbrooks-slackcontext-tabletestcommand-expect…

3ea5fa0

…edasserts-background-context

refactor: update root.go use to ctx for OnInitialize and OnFinalize

33440d9

mwbrooks marked this pull request as ready for review April 11, 2025 19:28

mwbrooks requested a review from a team as a code owner April 11, 2025 19:28

mwbrooks commented Apr 11, 2025

View reviewed changes

zimeg approved these changes Apr 11, 2025

View reviewed changes

mwbrooks added 2 commits April 14, 2025 11:50

refactor: pass ctx into Init

9f25031

Merge branch 'main' into mbrooks-slackcontext-tabletestcommand-expect…

94a3fdf

…edasserts-background-context

mwbrooks merged commit 85749e2 into main Apr 14, 2025
6 checks passed

mwbrooks deleted the mbrooks-slackcontext-tabletestcommand-expectedasserts-background-context branch April 14, 2025 20:35

refactor: update .Context() references for consistency #33

refactor: update .Context() references for consistency #33

Uh oh!

Conversation

mwbrooks commented Apr 11, 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 11, 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!

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

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 Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mwbrooks commented Apr 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwbrooks commented Apr 11, 2025 •

edited

Loading

codecov bot commented Apr 11, 2025 •

edited

Loading

mwbrooks Apr 11, 2025 •

edited

Loading

mwbrooks Apr 11, 2025 •

edited

Loading