Skip to content

Commit 0faf469

Browse files
zimegmwbrooks
zimeg
and
mwbrooks
authored
fix: 'run' exiting on disconnected api errors when streaming activity logs (#132)
Co-authored-by: Michael Brooks <[email protected]>
1 parent 4d342d7 commit 0faf469

File tree

4 files changed

+102
-15
lines changed

4 files changed

+102
-15
lines changed

‎internal/api/activity.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,13 +112,13 @@ func (c *Client) Activity(ctx context.Context, token string, activityRequest typ
112112

113113
b, err := c.get(ctx, url, token, "")
114114
if err != nil {
115-
return ActivityResult{}, errHTTPRequestFailed.WithRootCause(err)
115+
return ActivityResult{}, slackerror.New(slackerror.ErrHTTPRequestFailed).WithRootCause(err)
116116
}
117117

118118
resp := activityResponse{}
119119
err = goutils.JSONUnmarshal(b, &resp)
120120
if err != nil {
121-
return ActivityResult{}, errHTTPResponseInvalid.WithRootCause(err).AddAPIMethod(appActivityMethod)
121+
return ActivityResult{}, slackerror.New(slackerror.ErrHTTPResponseInvalid).WithRootCause(err).AddAPIMethod(appActivityMethod)
122122
}
123123

124124
if !resp.Ok {

‎internal/api/client.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,18 @@ type responseMetadata struct {
6969
}
7070

7171
var (
72-
errInvalidArguments = slackerror.New(slackerror.ErrInvalidArguments)
72+
// FIXME: errInvalidArguments should be returned as a new error from within
73+
// the API method call to avoid changing values for this error pointer across
74+
// API calls.
75+
errInvalidArguments = slackerror.New(slackerror.ErrInvalidArguments)
76+
// FIXME: errHTTPResponseInvalid should be returned as a new error right after
77+
// the API .JSONUnmarshal function call to avoid changing values for this error
78+
// pointer across API calls.
7379
errHTTPResponseInvalid = slackerror.New(slackerror.ErrHTTPResponseInvalid)
74-
errHTTPRequestFailed = slackerror.New(slackerror.ErrHTTPRequestFailed)
80+
// FIXME: errHTTPRequestFailed should be returned as a new error right after
81+
// the API .get call methods to avoid changing values for this error pointer
82+
// across API calls.
83+
errHTTPRequestFailed = slackerror.New(slackerror.ErrHTTPRequestFailed)
7584
)
7685

7786
// NewClient accepts an httpClient to facilitate making http requests to Slack.

‎internal/pkg/platform/activity.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,11 +88,12 @@ func Activity(
8888
}
8989

9090
latestCreatedTimestamp, _, err := printLatestActivity(ctx, clients, token, activityRequest, token)
91-
if err != nil {
92-
return err
93-
}
9491

92+
// Return an error from the activity API if the logs are not being tailed
9593
if !args.TailArg {
94+
if err != nil {
95+
return err
96+
}
9697
return nil
9798
}
9899

@@ -108,9 +109,11 @@ func Activity(
108109
case <-ticker.C:
109110
// Try to grab new logs using the last logs timestamp
110111
activityRequest.MinimumDateCreated = latestCreatedTimestamp + 1
112+
113+
// Avoid exit on error from the activity API when tailing logs
111114
newLatestCreatedTimestamp, count, err := printLatestActivity(ctx, clients, token, activityRequest, token)
112115
if err != nil {
113-
return slackerror.New(slackerror.ErrStreamingActivityLogs).WithRootCause(err)
116+
clients.IO.PrintDebug(ctx, "%s\n", err)
114117
}
115118

116119
if count > 0 {

‎internal/pkg/platform/activity_test.go‎

Lines changed: 82 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -110,28 +110,80 @@ func TestPlatformActivity_StreamingLogs(t *testing.T) {
110110
},
111111
"should return error if session validation fails": {
112112
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
113-
cm.API.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{}, slackerror.New("boomsies"))
113+
cm.API.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{}, slackerror.New("mock_broken_validation"))
114114
return ctx
115115
},
116-
ExpectedError: slackerror.New("boomsies"),
116+
ExpectedError: slackerror.New("mock_broken_validation"),
117117
},
118-
"should return error if activity request fails": {
118+
"should return error from Activity API request if TailArg is not set": {
119+
Args: types.ActivityArgs{
120+
TailArg: false,
121+
},
119122
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
120-
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, slackerror.New("explosions"))
123+
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, slackerror.New("mock_broken_logs"))
121124
return ctx
122125
},
123-
ExpectedError: slackerror.New("explosions"),
126+
ExpectedError: slackerror.New("mock_broken_logs"),
127+
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
128+
cm.API.AssertNumberOfCalls(t, "Activity", 1)
129+
},
124130
},
125131
"should return nil and invoke Activity API only once if TailArg is not set": {
126132
Args: types.ActivityArgs{
127-
TailArg: false,
133+
TailArg: false,
134+
IdleTimeoutM: 1,
135+
PollingIntervalMS: 20, // poll activity every 20 ms
128136
},
129137
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
130-
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, nil)
138+
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).
139+
Return(api.ActivityResult{
140+
Activities: []api.Activity{
141+
{
142+
Level: types.WARN,
143+
ComponentID: "a123",
144+
TraceID: "tr123",
145+
},
146+
{
147+
Level: types.ERROR,
148+
ComponentID: "a456",
149+
TraceID: "tr456",
150+
},
151+
},
152+
}, nil)
153+
ctx, cancel := context.WithCancel(ctx)
154+
go func() {
155+
time.Sleep(time.Millisecond * 50) // cancel activity in 50 ms
156+
cancel()
157+
}()
131158
return ctx
132159
},
160+
ExpectedError: nil,
133161
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
162+
// with the above tail argument not included we call the method once.
134163
cm.API.AssertNumberOfCalls(t, "Activity", 1)
164+
assert.Contains(t, cm.GetStdoutOutput(), "[warn] [a123] (Trace=tr123)")
165+
assert.Contains(t, cm.GetStdoutOutput(), "[error] [a456] (Trace=tr456)")
166+
},
167+
},
168+
"should return nil and invoke Activity API twice if TailArg is set while polling": {
169+
Args: types.ActivityArgs{
170+
TailArg: true,
171+
IdleTimeoutM: 0,
172+
PollingIntervalMS: 20, // poll activity every 20 ms
173+
},
174+
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
175+
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, nil)
176+
ctx, cancel := context.WithCancel(ctx)
177+
go func() {
178+
time.Sleep(time.Millisecond * 50) // cancel activity in 50 ms
179+
cancel()
180+
}()
181+
return ctx
182+
},
183+
ExpectedError: nil,
184+
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
185+
// with the above polling/canceling setup, expectation is activity called two times.
186+
cm.API.AssertNumberOfCalls(t, "Activity", 2)
135187
},
136188
},
137189
"should return nil if TailArg is set and context is canceled": {
@@ -148,11 +200,33 @@ func TestPlatformActivity_StreamingLogs(t *testing.T) {
148200
}()
149201
return ctx
150202
},
203+
ExpectedError: nil,
151204
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
152205
// with the above polling/canceling setup, expectation is activity called only once.
153206
cm.API.AssertNumberOfCalls(t, "Activity", 1)
154207
},
155208
},
209+
"should return nil if TailArg is set and activity request fails while polling": {
210+
Args: types.ActivityArgs{
211+
TailArg: true,
212+
IdleTimeoutM: 1,
213+
PollingIntervalMS: 20, // poll activity every 20 ms
214+
},
215+
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
216+
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, slackerror.New("mock_broken_logs"))
217+
ctx, cancel := context.WithCancel(ctx)
218+
go func() {
219+
time.Sleep(time.Millisecond * 50) // cancel activity in 50 ms
220+
cancel()
221+
}()
222+
return ctx
223+
},
224+
ExpectedError: nil,
225+
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
226+
// with the above polling/canceling setup, expectation is activity called three times.
227+
cm.API.AssertNumberOfCalls(t, "Activity", 3)
228+
},
229+
},
156230
} {
157231
t.Run(name, func(t *testing.T) {
158232
// Create mocks
@@ -172,6 +246,7 @@ func TestPlatformActivity_StreamingLogs(t *testing.T) {
172246

173247
err := Activity(ctxMock, clients, &logger.Logger{}, tt.Args)
174248
if tt.ExpectedError != nil {
249+
require.Error(t, err)
175250
assert.Contains(t, err.Error(), tt.ExpectedError.Error(), err)
176251
} else {
177252
require.NoError(t, err)

0 commit comments

Comments
 (0)