Skip to content

Commit b82e4c3

Browse files
mwbrooksmwbrooks
authored
refactor: Add slackcontext package to DRY context management (#7)
* refactor: Add slackcontext package to DRY context management * refactor: Revert changes to unrelated error codes
1 parent 02eeb55 commit b82e4c3

33 files changed

+887
-300
lines changed

‎.vscode/settings.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
"iostreams",
1717
"opentracing",
1818
"safeexec",
19+
"slackcontext",
1920
"slackdeps",
2021
"slackerror",
2122
"slackhq",

‎cmd/env/list.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func NewEnvListCommand(clients *shared.ClientFactory) *cobra.Command {
5151
return preRunEnvListCommandFunc(ctx, clients)
5252
},
5353
RunE: func(cmd *cobra.Command, args []string) error {
54-
return runEnvListCommandFunc(clients)
54+
return runEnvListCommandFunc(clients, cmd)
5555
},
5656
}
5757

@@ -74,8 +74,9 @@ func preRunEnvListCommandFunc(ctx context.Context, clients *shared.ClientFactory
7474
// runEnvListCommandFunc outputs environment variables for a selected app
7575
func runEnvListCommandFunc(
7676
clients *shared.ClientFactory,
77+
cmd *cobra.Command,
7778
) error {
78-
ctx := context.Background()
79+
ctx := cmd.Context()
7980

8081
selection, err := teamAppSelectPromptFunc(
8182
ctx,

‎cmd/root.go‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
"strings"
2323
"syscall"
2424

25-
"github.com/opentracing/opentracing-go"
2625
"github.com/slackapi/slack-cli/cmd/app"
2726
"github.com/slackapi/slack-cli/cmd/auth"
2827
"github.com/slackapi/slack-cli/cmd/collaborators"
@@ -47,6 +46,7 @@ import (
4746
"github.com/slackapi/slack-cli/internal/iostreams"
4847
"github.com/slackapi/slack-cli/internal/pkg/version"
4948
"github.com/slackapi/slack-cli/internal/shared"
49+
"github.com/slackapi/slack-cli/internal/slackcontext"
5050
"github.com/slackapi/slack-cli/internal/slackerror"
5151
"github.com/slackapi/slack-cli/internal/style"
5252
"github.com/slackapi/slack-cli/internal/update"
@@ -222,6 +222,7 @@ func Init() (*cobra.Command, *shared.ClientFactory) {
222222
// TODO: consider using arguments for this function for certain dependencies, like working directory and other OS-specific strings, that OnInitialize above can provide during actual execution, but that we can override with test values for easier testing.
223223
func InitConfig(clients *shared.ClientFactory, rootCmd *cobra.Command) error {
224224
ctx := rootCmd.Context()
225+
225226
// Get the current working directory (usually, but not always the project)
226227
workingDirPath, err := clients.Os.Getwd()
227228
if err != nil {
@@ -261,43 +262,35 @@ func InitConfig(clients *shared.ClientFactory, rootCmd *cobra.Command) error {
261262
clients.Config.ApiHostResolved = clients.AuthInterface().ResolveApiHost(ctx, clients.Config.ApiHostFlag, nil)
262263
clients.Config.LogstashHostResolved = clients.AuthInterface().ResolveLogstashHost(ctx, clients.Config.ApiHostResolved, clients.CliVersion)
263264

264-
// TODO: should we store system ID in config or in context?
265265
// Init System ID
266266
if systemID, err := clients.Config.SystemConfig.InitSystemID(ctx); err != nil {
267267
clients.IO.PrintDebug(ctx, "Error initializing user-level config system_id: %s", err.Error())
268268
} else {
269269
// Used by Logstash
270+
// TODO(slackcontext) Consolidate storing SystemID to slackcontext
270271
clients.Config.SystemID = systemID
271-
272272
// Used by OpenTracing
273-
if span := opentracing.SpanFromContext(ctx); span != nil {
274-
span.SetTag("system_id", clients.Config.SystemID)
275-
ctx = opentracing.ContextWithSpan(ctx, span)
276-
}
277-
273+
ctx = slackcontext.SetSystemID(ctx, systemID)
274+
rootCmd.SetContext(ctx)
278275
// Debug logging
279276
clients.IO.PrintDebug(ctx, "system_id: %s", clients.Config.SystemID)
280277
}
281278

282-
// TODO: should we store project ID in config or in context?
283279
// Init Project ID, if current directory is a project
284280
if projectID, _ := clients.Config.ProjectConfig.InitProjectID(ctx, false); projectID != "" {
285281
// Used by Logstash
282+
// TODO(slackcontext) Consolidate storing ProjectID to slackcontext
286283
clients.Config.ProjectID = projectID
287-
288284
// Used by OpenTracing
289-
if span := opentracing.SpanFromContext(ctx); span != nil {
290-
span.SetTag("project_id", clients.Config.ProjectID)
291-
ctx = opentracing.ContextWithSpan(ctx, span)
292-
}
293-
285+
ctx = slackcontext.SetProjectID(ctx, projectID)
286+
rootCmd.SetContext(ctx)
294287
// Debug logging
295288
clients.IO.PrintDebug(ctx, "project_id: %s", clients.Config.ProjectID)
296289
}
297290

298291
// Init configurations
299292
clients.Config.LoadExperiments(ctx, clients.IO.PrintDebug)
300-
// TODO: consolidate where we store CLI version; here are two locations, plus some code uses `contextutil.ContextFromVersion`, plus pkg/version/version
293+
// TODO(slackcontext) Consolidate storing CLI version to slackcontext
301294
clients.Config.Version = clients.CliVersion
302295

303296
// The domain auths (token->domain) shouldn't change for the execution of the CLI so preload them into config!
@@ -334,10 +327,8 @@ func InitConfig(clients *shared.ClientFactory, rootCmd *cobra.Command) error {
334327
// listens for process interrupts and sends to IOStreams' GetInterruptChannel() for use in
335328
// in communicating process interrupts elsewhere in the code.
336329
func Execute(ctx context.Context, rootCmd *cobra.Command, clients *shared.ClientFactory) {
337-
// TODO: ensure context is only used with setters and not with getters. After investigating, report:
338-
// - internal/contextutil/contextutil.go has a `VersionFromContext` getter method which is used in various API methods when setting the user agent on HTTP requests, and when sending data up to logstash
339-
// - internal/config/context.go has a variety of set and get methods related to app, token, team/enterprise/session/user/trace IDs, domains
340330
ctx, cancel := context.WithCancel(ctx)
331+
341332
completedChan := make(chan bool, 1) // completed is used for signalling an end to command
342333
exitChan := make(chan bool, 1) // exit blocks the command from exiting until completed
343334
interruptChan := make(chan os.Signal, 1) // interrupt catches signals to avoid abrupt exits
@@ -366,6 +357,7 @@ func Execute(ctx context.Context, rootCmd *cobra.Command, clients *shared.Client
366357
}()
367358
case <-ctx.Done():
368359
// No interrupt signal sent, command executed to completion
360+
// FIXME - `.Done()` channel is triggered by `cancel()` and not a successfully completion
369361
case <-completedChan:
370362
// No canceled context, but command has completed execution
371363
exitChan <- true

‎cmd/root_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,15 @@ import (
2121
"testing"
2222

2323
"github.com/slackapi/slack-cli/internal/shared"
24+
"github.com/slackapi/slack-cli/internal/slackcontext"
2425
"github.com/slackapi/slack-cli/test/testutil"
2526
"github.com/stretchr/testify/assert"
2627
"github.com/stretchr/testify/require"
2728
)
2829

2930
func TestRootCommand(t *testing.T) {
31+
ctx := slackcontext.MockContext(t.Context())
32+
3033
tmp, _ := os.MkdirTemp("", "")
3134
_ = os.Chdir(tmp)
3235
defer os.RemoveAll(tmp)
@@ -38,7 +41,7 @@ func TestRootCommand(t *testing.T) {
3841
clientsMock := shared.NewClientsMock()
3942
testutil.MockCmdIO(clientsMock.IO, cmd)
4043

41-
err := cmd.Execute()
44+
err := cmd.ExecuteContext(ctx)
4245
if err != nil {
4346
assert.Fail(t, "cmd.Execute had unexpected error")
4447
}
@@ -131,6 +134,7 @@ func Test_Aliases(t *testing.T) {
131134
tmp, _ := os.MkdirTemp("", "")
132135
_ = os.Chdir(tmp)
133136
defer os.RemoveAll(tmp)
137+
134138
Init()
135139

136140
tests := map[string]struct {

‎internal/api/activity_test.go‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@
1515
package api
1616

1717
import (
18-
"context"
1918
"testing"
2019

2120
"github.com/slackapi/slack-cli/internal/shared/types"
21+
"github.com/slackapi/slack-cli/internal/slackcontext"
2222
"github.com/slackapi/slack-cli/internal/slackerror"
2323
"github.com/stretchr/testify/require"
2424
)
@@ -28,39 +28,42 @@ var fakeResult = `{"ok":true,
2828
}`
2929

3030
func Test_ApiClient_ActivityErrorsIfAppIdIsEmpty(t *testing.T) {
31+
ctx := slackcontext.MockContext(t.Context())
3132
c, teardown := NewFakeClient(t, FakeClientParams{
3233
ExpectedMethod: appActivityMethod,
3334
})
3435
defer teardown()
35-
_, err := c.Activity(context.Background(), "token", types.ActivityRequest{
36+
_, err := c.Activity(ctx, "token", types.ActivityRequest{
3637
AppId: "",
3738
})
3839
require.Error(t, err)
3940
require.Contains(t, err.Error(), "app is not deployed")
4041
}
4142

4243
func Test_ApiClient_ActivityBasicSuccessfulGET(t *testing.T) {
44+
ctx := slackcontext.MockContext(t.Context())
4345
c, teardown := NewFakeClient(t, FakeClientParams{
4446
ExpectedMethod: appActivityMethod,
4547
ExpectedQuerystring: "app_id=A123",
4648
Response: fakeResult,
4749
})
4850
defer teardown()
49-
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
51+
result, err := c.Activity(ctx, "token", types.ActivityRequest{
5052
AppId: "A123",
5153
})
5254
require.NoError(t, err)
5355
require.Equal(t, result.Activities[0].TraceId, "12345")
5456
}
5557

5658
func Test_ApiClient_ActivityEventType(t *testing.T) {
59+
ctx := slackcontext.MockContext(t.Context())
5760
c, teardown := NewFakeClient(t, FakeClientParams{
5861
ExpectedMethod: appActivityMethod,
5962
ExpectedQuerystring: "app_id=A123&limit=0&log_event_type=silly",
6063
Response: fakeResult,
6164
})
6265
defer teardown()
63-
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
66+
result, err := c.Activity(ctx, "token", types.ActivityRequest{
6467
AppId: "A123",
6568
EventType: "silly",
6669
})
@@ -69,13 +72,14 @@ func Test_ApiClient_ActivityEventType(t *testing.T) {
6972
}
7073

7174
func Test_ApiClient_ActivityLogLevel(t *testing.T) {
75+
ctx := slackcontext.MockContext(t.Context())
7276
c, teardown := NewFakeClient(t, FakeClientParams{
7377
ExpectedMethod: appActivityMethod,
7478
ExpectedQuerystring: "app_id=A123&limit=0&min_log_level=silly",
7579
Response: fakeResult,
7680
})
7781
defer teardown()
78-
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
82+
result, err := c.Activity(ctx, "token", types.ActivityRequest{
7983
AppId: "A123",
8084
MinimumLogLevel: "silly",
8185
})
@@ -84,13 +88,14 @@ func Test_ApiClient_ActivityLogLevel(t *testing.T) {
8488
}
8589

8690
func Test_ApiClient_ActivityMinDateCreated(t *testing.T) {
91+
ctx := slackcontext.MockContext(t.Context())
8792
c, teardown := NewFakeClient(t, FakeClientParams{
8893
ExpectedMethod: appActivityMethod,
8994
ExpectedQuerystring: "app_id=A123&limit=0&min_date_created=1337",
9095
Response: fakeResult,
9196
})
9297
defer teardown()
93-
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
98+
result, err := c.Activity(ctx, "token", types.ActivityRequest{
9499
AppId: "A123",
95100
MinimumDateCreated: 1337,
96101
})
@@ -99,13 +104,14 @@ func Test_ApiClient_ActivityMinDateCreated(t *testing.T) {
99104
}
100105

101106
func Test_ApiClient_ActivityComponentType(t *testing.T) {
107+
ctx := slackcontext.MockContext(t.Context())
102108
c, teardown := NewFakeClient(t, FakeClientParams{
103109
ExpectedMethod: appActivityMethod,
104110
ExpectedQuerystring: "app_id=A123&limit=0&component_type=defirbulator",
105111
Response: fakeResult,
106112
})
107113
defer teardown()
108-
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
114+
result, err := c.Activity(ctx, "token", types.ActivityRequest{
109115
AppId: "A123",
110116
ComponentType: "defirbulator",
111117
})
@@ -114,13 +120,14 @@ func Test_ApiClient_ActivityComponentType(t *testing.T) {
114120
}
115121

116122
func Test_ApiClient_ActivityComponentId(t *testing.T) {
123+
ctx := slackcontext.MockContext(t.Context())
117124
c, teardown := NewFakeClient(t, FakeClientParams{
118125
ExpectedMethod: appActivityMethod,
119126
ExpectedQuerystring: "app_id=A123&limit=0&component_id=raspberry",
120127
Response: fakeResult,
121128
})
122129
defer teardown()
123-
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
130+
result, err := c.Activity(ctx, "token", types.ActivityRequest{
124131
AppId: "A123",
125132
ComponentId: "raspberry",
126133
})
@@ -129,13 +136,14 @@ func Test_ApiClient_ActivityComponentId(t *testing.T) {
129136
}
130137

131138
func Test_ApiClient_ActivitySource(t *testing.T) {
139+
ctx := slackcontext.MockContext(t.Context())
132140
c, teardown := NewFakeClient(t, FakeClientParams{
133141
ExpectedMethod: appActivityMethod,
134142
ExpectedQuerystring: "app_id=A123&limit=0&source=beer",
135143
Response: fakeResult,
136144
})
137145
defer teardown()
138-
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
146+
result, err := c.Activity(ctx, "token", types.ActivityRequest{
139147
AppId: "A123",
140148
Source: "beer",
141149
})
@@ -144,13 +152,14 @@ func Test_ApiClient_ActivitySource(t *testing.T) {
144152
}
145153

146154
func Test_ApiClient_ActivityTraceId(t *testing.T) {
155+
ctx := slackcontext.MockContext(t.Context())
147156
c, teardown := NewFakeClient(t, FakeClientParams{
148157
ExpectedMethod: appActivityMethod,
149158
ExpectedQuerystring: "app_id=A123&limit=0&trace_id=stealth",
150159
Response: fakeResult,
151160
})
152161
defer teardown()
153-
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
162+
result, err := c.Activity(ctx, "token", types.ActivityRequest{
154163
AppId: "A123",
155164
TraceId: "stealth",
156165
})
@@ -159,41 +168,44 @@ func Test_ApiClient_ActivityTraceId(t *testing.T) {
159168
}
160169

161170
func Test_ApiClient_ActivityResponseNotOK(t *testing.T) {
171+
ctx := slackcontext.MockContext(t.Context())
162172
c, teardown := NewFakeClient(t, FakeClientParams{
163173
ExpectedMethod: appActivityMethod,
164174
ExpectedQuerystring: "app_id=A123",
165175
Response: `{"ok":false, "error": "internal_error"}`,
166176
})
167177
defer teardown()
168-
_, err := c.Activity(context.Background(), "token", types.ActivityRequest{
178+
_, err := c.Activity(ctx, "token", types.ActivityRequest{
169179
AppId: "A123",
170180
})
171181
require.Error(t, err)
172182
require.Contains(t, err.Error(), "internal_error")
173183
}
174184

175185
func Test_ApiClient_ActivityInvalidResponse(t *testing.T) {
186+
ctx := slackcontext.MockContext(t.Context())
176187
c, teardown := NewFakeClient(t, FakeClientParams{
177188
ExpectedMethod: appActivityMethod,
178189
ExpectedQuerystring: "app_id=A123",
179190
Response: `badjson`,
180191
})
181192
defer teardown()
182-
_, err := c.Activity(context.Background(), "token", types.ActivityRequest{
193+
_, err := c.Activity(ctx, "token", types.ActivityRequest{
183194
AppId: "A123",
184195
})
185196
require.Error(t, err)
186197
require.Contains(t, err.Error(), slackerror.ErrHttpResponseInvalid)
187198
}
188199

189200
func Test_ApiClient_ActivityInvalidJSON(t *testing.T) {
201+
ctx := slackcontext.MockContext(t.Context())
190202
c, teardown := NewFakeClient(t, FakeClientParams{
191203
ExpectedMethod: appActivityMethod,
192204
ExpectedQuerystring: "app_id=A123",
193205
Response: `badtime`,
194206
})
195207
defer teardown()
196-
_, err := c.Activity(context.Background(), "token", types.ActivityRequest{
208+
_, err := c.Activity(ctx, "token", types.ActivityRequest{
197209
AppId: "A123",
198210
})
199211
require.Error(t, err)

0 commit comments

Comments
 (0)