-
Notifications
You must be signed in to change notification settings - Fork 24
refactor: update .Context() references for consistency #33
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.
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
Changes from all commits
4add4cc
6c8907a
f7b9014
b8f68e9
9c816be
5f69d23
c4b7697
63a99c8
ad91292
158b3f6
47a5bcd
ca08f10
3ea5fa0
33440d9
9f25031
94a3fdf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,7 +61,7 @@ func Test_Apps_Link(t *testing.T) { | |
| mockLinkSlackAuth1, | ||
| }, nil) | ||
| cm.AddDefaultMocks() | ||
| setupAppLinkCommandMocks(t, cm, cf) | ||
| setupAppLinkCommandMocks(t, ctx, cm, cf) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: This catches a |
||
| cm.IO.On("SelectPrompt", | ||
| mock.Anything, | ||
| "Select the existing app team", | ||
|
|
@@ -118,7 +118,7 @@ func Test_Apps_Link(t *testing.T) { | |
| mockLinkSlackAuth1, | ||
| }, nil) | ||
| cm.AddDefaultMocks() | ||
| setupAppLinkCommandMocks(t, cm, cf) | ||
| setupAppLinkCommandMocks(t, ctx, cm, cf) | ||
| cm.IO.On("SelectPrompt", | ||
| mock.Anything, | ||
| "Select the existing app team", | ||
|
|
@@ -177,7 +177,7 @@ func Test_Apps_Link(t *testing.T) { | |
| mockLinkSlackAuth2, | ||
| }, nil) | ||
| cm.AddDefaultMocks() | ||
| setupAppLinkCommandMocks(t, cm, cf) | ||
| setupAppLinkCommandMocks(t, ctx, cm, cf) | ||
| existingApp := types.App{ | ||
| AppID: mockLinkAppID1, | ||
| TeamDomain: mockLinkSlackAuth1.TeamDomain, | ||
|
|
@@ -246,7 +246,7 @@ func Test_Apps_Link(t *testing.T) { | |
| mockLinkSlackAuth2, | ||
| }, nil) | ||
| cm.AddDefaultMocks() | ||
| setupAppLinkCommandMocks(t, cm, cf) | ||
| setupAppLinkCommandMocks(t, ctx, cm, cf) | ||
| existingApp := types.App{ | ||
| AppID: mockLinkAppID1, | ||
| TeamDomain: mockLinkSlackAuth1.TeamDomain, | ||
|
|
@@ -321,7 +321,7 @@ func Test_Apps_Link(t *testing.T) { | |
| mockLinkSlackAuth2, | ||
| }, nil) | ||
| cm.AddDefaultMocks() | ||
| setupAppLinkCommandMocks(t, cm, cf) | ||
| setupAppLinkCommandMocks(t, ctx, cm, cf) | ||
| existingApp := types.App{ | ||
| AppID: mockLinkAppID2, | ||
| TeamDomain: mockLinkSlackAuth2.TeamDomain, | ||
|
|
@@ -389,7 +389,7 @@ func Test_Apps_Link(t *testing.T) { | |
| mockLinkSlackAuth2, | ||
| }, nil) | ||
| cm.AddDefaultMocks() | ||
| setupAppLinkCommandMocks(t, cm, cf) | ||
| setupAppLinkCommandMocks(t, ctx, cm, cf) | ||
| cm.IO.On("SelectPrompt", | ||
| mock.Anything, | ||
| "Select the existing app team", | ||
|
|
@@ -437,9 +437,9 @@ func Test_Apps_Link(t *testing.T) { | |
| mockLinkSlackAuth1, | ||
| }, nil) | ||
| cm.AddDefaultMocks() | ||
| setupAppLinkCommandMocks(t, cm, cf) | ||
| setupAppLinkCommandMocks(t, ctx, cm, cf) | ||
| // Set manifest source to project to trigger confirmation prompt | ||
| if err := cm.Config.ProjectConfig.SetManifestSource(t.Context(), config.MANIFEST_SOURCE_LOCAL); err != nil { | ||
| if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.MANIFEST_SOURCE_LOCAL); err != nil { | ||
| require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err)) | ||
| } | ||
| // Accept manifest source confirmation prompt | ||
|
|
@@ -506,9 +506,9 @@ func Test_Apps_Link(t *testing.T) { | |
| "decline manifest source prompt should not link app": { | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cm.AddDefaultMocks() | ||
| setupAppLinkCommandMocks(t, cm, cf) | ||
| setupAppLinkCommandMocks(t, ctx, cm, cf) | ||
| // Set manifest source to project to trigger confirmation prompt | ||
| if err := cm.Config.ProjectConfig.SetManifestSource(t.Context(), config.MANIFEST_SOURCE_LOCAL); err != nil { | ||
| if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.MANIFEST_SOURCE_LOCAL); err != nil { | ||
| require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err)) | ||
| } | ||
| // Decline manifest source confirmation prompt | ||
|
|
@@ -593,8 +593,7 @@ func Test_Apps_LinkAppHeaderSection(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func setupAppLinkCommandMocks(t *testing.T, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| ctx := t.Context() | ||
| func setupAppLinkCommandMocks(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| projectDirPath := slackdeps.MockWorkingDirectory | ||
| cm.Os.On("Getwd").Return(projectDirPath, nil) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,9 +74,8 @@ func NewUpdateCommand(clients *shared.ClientFactory) *cobra.Command { | |
|
|
||
| // runUpdateCommand will execute the update command | ||
| func runUpdateCommand(cmd *cobra.Command, clients *shared.ClientFactory, args []string) error { | ||
| var span opentracing.Span | ||
| ctx := cmd.Context() | ||
| span, ctx = opentracing.StartSpanFromContext(ctx, "cmd.Collaborators.Update") | ||
| span, _ := opentracing.StartSpanFromContext(ctx, "cmd.Collaborators.Update") | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mwbrooks Thanks for calling this out. How we use 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 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making this more consistent overall is a nice change in this PR too!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, you bring up a good point about the 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. |
||
| defer span.Finish() | ||
|
|
||
| var slackUser types.SlackUser | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ctxensuring more consistency through the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ctxas 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 😉