Skip to content

refactor: Add slackcontext package to DRY context management #7

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 4 commits into from
Apr 8, 2025
Merged

refactor: Add slackcontext package to DRY context management #7

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4add4cc
refactor: Add slackcontext package to DRY context management
mwbrooks Apr 2, 2025
6c8907a
refactor: Revert changes to unrelated error codes
mwbrooks Apr 2, 2025
aae77de
Merge branch 'main' into mbrooks-slackcontext
mwbrooks Apr 3, 2025
a16ba4c
Merge branch 'main' into mbrooks-slackcontext
mwbrooks Apr 8, 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
1 change: 1 addition & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"iostreams",
"opentracing",
"safeexec",
"slackcontext",
"slackdeps",
"slackerror",
"slackhq",
Expand Down
5 changes: 3 additions & 2 deletions cmd/env/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func NewEnvListCommand(clients *shared.ClientFactory) *cobra.Command {
return preRunEnvListCommandFunc(ctx, clients)
},
RunE: func(cmd *cobra.Command, args []string) error {
return runEnvListCommandFunc(clients)
return runEnvListCommandFunc(clients, cmd)
Copy link
Member Author

@mwbrooks mwbrooks Apr 2, 2025

Choose a reason for hiding this comment

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

While runEnvListCommandFunc(ctx, clients, cmd) would be a more appropriate format, the other env commands only pass cmd and fetch the context with cmd.Context().

This change is leaning on consistency and a smaller change footprint, since the PR is already quite large.

},
}

Expand All @@ -74,8 +74,9 @@ func preRunEnvListCommandFunc(ctx context.Context, clients *shared.ClientFactory
// runEnvListCommandFunc outputs environment variables for a selected app
func runEnvListCommandFunc(
clients *shared.ClientFactory,
cmd *cobra.Command,
) error {
ctx := context.Background()
ctx := cmd.Context()
Copy link
Member Author

@mwbrooks mwbrooks Apr 2, 2025

Choose a reason for hiding this comment

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

Fixed getting the real context, so that the env list command can access the Slack CLI Version. Currently, all API requests for the env list command don't include the Version in the user-agent.


selection, err := teamAppSelectPromptFunc(
ctx,
Expand Down
30 changes: 11 additions & 19 deletions cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"strings"
"syscall"

"github.com/opentracing/opentracing-go"
"github.com/slackapi/slack-cli/cmd/app"
"github.com/slackapi/slack-cli/cmd/auth"
"github.com/slackapi/slack-cli/cmd/collaborators"
Expand All @@ -47,6 +46,7 @@ import (
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/pkg/version"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/style"
"github.com/slackapi/slack-cli/internal/update"
Expand Down Expand Up @@ -222,6 +222,7 @@ func Init() (*cobra.Command, *shared.ClientFactory) {
// 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.
func InitConfig(clients *shared.ClientFactory, rootCmd *cobra.Command) error {
ctx := rootCmd.Context()

// Get the current working directory (usually, but not always the project)
workingDirPath, err := clients.Os.Getwd()
if err != nil {
Expand Down Expand Up @@ -261,43 +262,35 @@ func InitConfig(clients *shared.ClientFactory, rootCmd *cobra.Command) error {
clients.Config.ApiHostResolved = clients.AuthInterface().ResolveApiHost(ctx, clients.Config.ApiHostFlag, nil)
clients.Config.LogstashHostResolved = clients.AuthInterface().ResolveLogstashHost(ctx, clients.Config.ApiHostResolved, clients.CliVersion)

// TODO: should we store system ID in config or in context?
// Init System ID
if systemID, err := clients.Config.SystemConfig.InitSystemID(ctx); err != nil {
clients.IO.PrintDebug(ctx, "Error initializing user-level config system_id: %s", err.Error())
} else {
// Used by Logstash
// TODO(slackcontext) Consolidate storing SystemID to slackcontext
clients.Config.SystemID = systemID
Comment on lines +270 to 271
Copy link
Member Author

@mwbrooks mwbrooks Apr 2, 2025

Choose a reason for hiding this comment

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

This would be a larger refactor that can wait until we 🪓 axe storing state in clients.Config


// Used by OpenTracing
if span := opentracing.SpanFromContext(ctx); span != nil {
span.SetTag("system_id", clients.Config.SystemID)
ctx = opentracing.ContextWithSpan(ctx, span)
}

ctx = slackcontext.SetSystemID(ctx, systemID)
rootCmd.SetContext(ctx)
// Debug logging
clients.IO.PrintDebug(ctx, "system_id: %s", clients.Config.SystemID)
}

// TODO: should we store project ID in config or in context?
// Init Project ID, if current directory is a project
if projectID, _ := clients.Config.ProjectConfig.InitProjectID(ctx, false); projectID != "" {
// Used by Logstash
// TODO(slackcontext) Consolidate storing ProjectID to slackcontext
clients.Config.ProjectID = projectID
Comment on lines +282 to 283
Copy link
Member Author

@mwbrooks mwbrooks Apr 2, 2025

Choose a reason for hiding this comment

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

Likewise, this can wait until we 🪓 axe storing state in clients.Config


// Used by OpenTracing
if span := opentracing.SpanFromContext(ctx); span != nil {
span.SetTag("project_id", clients.Config.ProjectID)
ctx = opentracing.ContextWithSpan(ctx, span)
}

ctx = slackcontext.SetProjectID(ctx, projectID)
rootCmd.SetContext(ctx)
Comment on lines -289 to +286
Copy link
Member

@zimeg zimeg Apr 8, 2025

Choose a reason for hiding this comment

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

👾 ✨

Copy link
Member Author

@mwbrooks mwbrooks Apr 8, 2025

Choose a reason for hiding this comment

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

I don't think we need to do this, because the rootCmd.ExecuteContext(ctx) will execute the command with our context. However, if someone called rootCmd.Context() before executing the Cobra command, they'll at least get the correct context.

// Debug logging
clients.IO.PrintDebug(ctx, "project_id: %s", clients.Config.ProjectID)
}

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

// The domain auths (token->domain) shouldn't change for the execution of the CLI so preload them into config!
Expand Down Expand Up @@ -334,10 +327,8 @@ func InitConfig(clients *shared.ClientFactory, rootCmd *cobra.Command) error {
// listens for process interrupts and sends to IOStreams' GetInterruptChannel() for use in
// in communicating process interrupts elsewhere in the code.
func Execute(ctx context.Context, rootCmd *cobra.Command, clients *shared.ClientFactory) {
// TODO: ensure context is only used with setters and not with getters. After investigating, report:
// - 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
// - internal/config/context.go has a variety of set and get methods related to app, token, team/enterprise/session/user/trace IDs, domains
Comment on lines -337 to -339
Copy link
Member Author

@mwbrooks mwbrooks Apr 2, 2025

Choose a reason for hiding this comment

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

These files have been refactored to slackcontext and the comment is no longer relevant.

ctx, cancel := context.WithCancel(ctx)

completedChan := make(chan bool, 1) // completed is used for signalling an end to command
exitChan := make(chan bool, 1) // exit blocks the command from exiting until completed
interruptChan := make(chan os.Signal, 1) // interrupt catches signals to avoid abrupt exits
Expand Down Expand Up @@ -366,6 +357,7 @@ func Execute(ctx context.Context, rootCmd *cobra.Command, clients *shared.Client
}()
case <-ctx.Done():
// No interrupt signal sent, command executed to completion
// FIXME - `.Done()` channel is triggered by `cancel()` and not a successfully completion
Copy link
Member Author

@mwbrooks mwbrooks Apr 2, 2025

Choose a reason for hiding this comment

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

This is a bug that i noticed while learning more about context. The .Done() is misleading - it's called when the context is cancelled NOT when the execution is complete.

Copy link
Member

@zimeg zimeg Apr 8, 2025

Choose a reason for hiding this comment

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

I'm super curious about this 👀

It's not super clear what the bug is but I'm not doubting that this might be causing problems... Were you noticing something strange during command completion?

Copy link
Member Author

@mwbrooks mwbrooks Apr 8, 2025

Choose a reason for hiding this comment

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

I only noticed the "bug" 🐛 while reading the code and didn't test it.

From my understanding, the commenter thought ctx.Done() is called when the execution is completed, but it's actually called if ctx.Cancel() is called. I don't see any references to ctx.Cancel() so this switch case is never actually met. Regardless, if it was met, it falls through to the successful completion switch case. So functionally, everything would work fine.

So, the main bug is just the comment that's misleading. I'll make a note to follow-up on this and check if we should be cancelling the context at some point 🤔

case <-completedChan:
// No canceled context, but command has completed execution
exitChan <- true
Expand Down
6 changes: 5 additions & 1 deletion cmd/root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,15 @@ import (
"testing"

"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/test/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestRootCommand(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())

Comment on lines +31 to +32
Copy link
Member Author

@mwbrooks mwbrooks Apr 2, 2025

Choose a reason for hiding this comment

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

I've introduced a slackcontext.MockContext(ctx) helper to setup a context with the guaranteed, default values set before a command executes.

Copy link
Member

@zimeg zimeg Apr 8, 2025

Choose a reason for hiding this comment

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

@mwbrooks This is wonderful with the setup in root!

slack-cli/internal/slackcontext/slackcontext_mock.go

Lines 23 to 34 in aae77de

const (
mockCLIVersion = "v1.2.3"
)
// MockContext sets values in the context that are guaranteed to exist before
// the Cobra root command is executed.
func MockContext(ctx context.Context) context.Context {
ctx = SetOpenTracingTraceID(ctx, uuid.New().String())
ctx = SetSessionID(ctx, uuid.New().String())
ctx = SetVersion(ctx, mockCLIVersion)
return ctx
}

tmp, _ := os.MkdirTemp("", "")
_ = os.Chdir(tmp)
defer os.RemoveAll(tmp)
Expand All @@ -38,7 +41,7 @@ func TestRootCommand(t *testing.T) {
clientsMock := shared.NewClientsMock()
testutil.MockCmdIO(clientsMock.IO, cmd)

err := cmd.Execute()
err := cmd.ExecuteContext(ctx)
Comment on lines -41 to +44
Copy link
Member

@zimeg zimeg Apr 8, 2025

Choose a reason for hiding this comment

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

TIL! Is this the same as Execute, but sets the ctx on the command?

Edit: Just saw the note in the PR description - this is neat! 🙏 📚

Copy link
Member Author

@mwbrooks mwbrooks Apr 8, 2025

Choose a reason for hiding this comment

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

You got it!

  • cmd.Execute() will use a context.Background() (empty context)
  • cmd.ExecuteContext(ctx) will use ctx

Whenever someone called cmd.Context() they'll get whatever context has been set. We want to ensure that they always get our context.

if err != nil {
assert.Fail(t, "cmd.Execute had unexpected error")
}
Expand Down Expand Up @@ -131,6 +134,7 @@ func Test_Aliases(t *testing.T) {
tmp, _ := os.MkdirTemp("", "")
_ = os.Chdir(tmp)
defer os.RemoveAll(tmp)

Init()

tests := map[string]struct {
Expand Down
38 changes: 25 additions & 13 deletions internal/api/activity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
package api

import (
"context"
"testing"

"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/stretchr/testify/require"
)
Expand All @@ -28,39 +28,42 @@ var fakeResult = `{"ok":true,
}`

func Test_ApiClient_ActivityErrorsIfAppIdIsEmpty(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
})
defer teardown()
_, err := c.Activity(context.Background(), "token", types.ActivityRequest{
_, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "",
})
require.Error(t, err)
require.Contains(t, err.Error(), "app is not deployed")
}

func Test_ApiClient_ActivityBasicSuccessfulGET(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123",
Response: fakeResult,
})
defer teardown()
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
result, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
})
require.NoError(t, err)
require.Equal(t, result.Activities[0].TraceId, "12345")
}

func Test_ApiClient_ActivityEventType(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123&limit=0&log_event_type=silly",
Response: fakeResult,
})
defer teardown()
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
result, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
EventType: "silly",
})
Expand All @@ -69,13 +72,14 @@ func Test_ApiClient_ActivityEventType(t *testing.T) {
}

func Test_ApiClient_ActivityLogLevel(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123&limit=0&min_log_level=silly",
Response: fakeResult,
})
defer teardown()
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
result, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
MinimumLogLevel: "silly",
})
Expand All @@ -84,13 +88,14 @@ func Test_ApiClient_ActivityLogLevel(t *testing.T) {
}

func Test_ApiClient_ActivityMinDateCreated(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123&limit=0&min_date_created=1337",
Response: fakeResult,
})
defer teardown()
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
result, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
MinimumDateCreated: 1337,
})
Expand All @@ -99,13 +104,14 @@ func Test_ApiClient_ActivityMinDateCreated(t *testing.T) {
}

func Test_ApiClient_ActivityComponentType(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123&limit=0&component_type=defirbulator",
Response: fakeResult,
})
defer teardown()
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
result, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
ComponentType: "defirbulator",
})
Expand All @@ -114,13 +120,14 @@ func Test_ApiClient_ActivityComponentType(t *testing.T) {
}

func Test_ApiClient_ActivityComponentId(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123&limit=0&component_id=raspberry",
Response: fakeResult,
})
defer teardown()
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
result, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
ComponentId: "raspberry",
})
Expand All @@ -129,13 +136,14 @@ func Test_ApiClient_ActivityComponentId(t *testing.T) {
}

func Test_ApiClient_ActivitySource(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123&limit=0&source=beer",
Response: fakeResult,
})
defer teardown()
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
result, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
Source: "beer",
})
Expand All @@ -144,13 +152,14 @@ func Test_ApiClient_ActivitySource(t *testing.T) {
}

func Test_ApiClient_ActivityTraceId(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123&limit=0&trace_id=stealth",
Response: fakeResult,
})
defer teardown()
result, err := c.Activity(context.Background(), "token", types.ActivityRequest{
result, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
TraceId: "stealth",
})
Expand All @@ -159,41 +168,44 @@ func Test_ApiClient_ActivityTraceId(t *testing.T) {
}

func Test_ApiClient_ActivityResponseNotOK(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123",
Response: `{"ok":false, "error": "internal_error"}`,
})
defer teardown()
_, err := c.Activity(context.Background(), "token", types.ActivityRequest{
_, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
})
require.Error(t, err)
require.Contains(t, err.Error(), "internal_error")
}

func Test_ApiClient_ActivityInvalidResponse(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123",
Response: `badjson`,
})
defer teardown()
_, err := c.Activity(context.Background(), "token", types.ActivityRequest{
_, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
})
require.Error(t, err)
require.Contains(t, err.Error(), slackerror.ErrHttpResponseInvalid)
}

func Test_ApiClient_ActivityInvalidJSON(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appActivityMethod,
ExpectedQuerystring: "app_id=A123",
Response: `badtime`,
})
defer teardown()
_, err := c.Activity(context.Background(), "token", types.ActivityRequest{
_, err := c.Activity(ctx, "token", types.ActivityRequest{
AppId: "A123",
})
require.Error(t, err)
Expand Down
Loading
Loading