Skip to content

Commit a9c266f

Browse files
belimawrbelimawr
committed
Stop setting global state on tests
Stop calling `paths.SetTop()` in the diagnostics test. This refactoring makes arguments and fields instead of modifying the global state starting from the test in `internal/pkg/diagnostics/diagnostics_test.go` up to the cobra command.
1 parent 46b01dc commit a9c266f

10 files changed

+76
-47
lines changed

‎internal/pkg/agent/application/actions/handlers/handler_action_diagnostics.go

+9-3
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/elastic/elastic-agent/pkg/control/v2/client"
1717
"github.com/elastic/elastic-agent/pkg/control/v2/cproto"
1818

19+
"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
1920
"github.com/elastic/elastic-agent/internal/pkg/core/monitoring/config"
2021
"github.com/elastic/elastic-agent/internal/pkg/diagnostics"
2122
"github.com/elastic/elastic-agent/internal/pkg/fleetapi"
@@ -67,15 +68,20 @@ type Diagnostics struct {
6768
diagProvider diagnosticsProvider
6869
limiter *rate.Limiter
6970
uploader Uploader
71+
topPath string
7072
}
7173

7274
// NewDiagnostics returns a new Diagnostics handler.
73-
func NewDiagnostics(log abstractLogger, coord diagnosticsProvider, cfg config.Limit, uploader Uploader) *Diagnostics {
75+
func NewDiagnostics(log abstractLogger, topPath string, coord diagnosticsProvider, cfg config.Limit, uploader Uploader) *Diagnostics {
76+
if topPath == "" {
77+
topPath = paths.Top()
78+
}
7479
return &Diagnostics{
7580
log: log,
7681
diagProvider: coord,
7782
limiter: rate.NewLimiter(rate.Every(cfg.Interval), cfg.Burst),
7883
uploader: uploader,
84+
topPath: topPath,
7985
}
8086
}
8187

@@ -155,7 +161,7 @@ func (h *Diagnostics) collectDiag(ctx context.Context, action *fleetapi.ActionDi
155161
h.log.Warn(str)
156162
}
157163
}()
158-
err := diagnostics.ZipArchive(&wBuf, &b, aDiag, uDiag, cDiag, action.ExcludeEventsLog)
164+
err := diagnostics.ZipArchive(&wBuf, &b, h.topPath, aDiag, uDiag, cDiag, action.ExcludeEventsLog)
159165
if err != nil {
160166
h.log.Errorw(
161167
"diagnostics action handler failed generate zip archive",
@@ -344,7 +350,7 @@ func (h *Diagnostics) diagFile(
344350
h.log.Warn(str)
345351
}
346352
}()
347-
if err := diagnostics.ZipArchive(&wBuf, f, aDiag, uDiag, cDiag, excludeEvents); err != nil {
353+
if err := diagnostics.ZipArchive(&wBuf, f, h.topPath, aDiag, uDiag, cDiag, excludeEvents); err != nil {
348354
os.Remove(name)
349355
return nil, 0, err
350356
}

‎internal/pkg/agent/application/actions/handlers/handler_action_diagnostics_test.go

+7-9
Original file line numberDiff line numberDiff line change
@@ -75,16 +75,14 @@ var (
7575
)
7676

7777
func TestDiagnosticHandlerHappyPathWithLogs(t *testing.T) {
78-
7978
tempAgentRoot := t.TempDir()
80-
paths.SetTop(tempAgentRoot)
8179
err := os.MkdirAll(path.Join(tempAgentRoot, "data"), 0755)
8280
require.NoError(t, err)
8381

8482
mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
8583
mockUploader := mockhandlers.NewUploader(t)
8684
testLogger, observedLogs := logger.NewTesting("diagnostic-handler-test")
87-
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
85+
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)
8886

8987
mockDiagProvider.EXPECT().DiagnosticHooks().Return([]diagnostics.Hook{hook1})
9088
mockDiagProvider.EXPECT().PerformDiagnostics(mock.Anything, mock.Anything).Return([]runtime.ComponentUnitDiagnostic{mockUnitDiagnostic})
@@ -165,7 +163,7 @@ func TestDiagnosticHandlerUploaderErrorWithLogs(t *testing.T) {
165163
mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
166164
mockUploader := mockhandlers.NewUploader(t)
167165
testLogger, observedLogs := logger.NewTesting("diagnostic-handler-test")
168-
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
166+
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)
169167

170168
mockDiagProvider.EXPECT().DiagnosticHooks().Return([]diagnostics.Hook{})
171169
mockDiagProvider.EXPECT().PerformDiagnostics(mock.Anything, mock.Anything).Return([]runtime.ComponentUnitDiagnostic{})
@@ -206,7 +204,7 @@ func TestDiagnosticHandlerZipArchiveErrorWithLogs(t *testing.T) {
206204
mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
207205
mockUploader := mockhandlers.NewUploader(t)
208206
testLogger, observedLogs := logger.NewTesting("diagnostic-handler-test")
209-
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
207+
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)
210208

211209
mockDiagProvider.EXPECT().DiagnosticHooks().Return([]diagnostics.Hook{})
212210
mockDiagProvider.EXPECT().PerformDiagnostics(mock.Anything, mock.Anything).Return([]runtime.ComponentUnitDiagnostic{})
@@ -242,7 +240,7 @@ func TestDiagnosticHandlerAckErrorWithLogs(t *testing.T) {
242240
mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
243241
mockUploader := mockhandlers.NewUploader(t)
244242
testLogger, observedLogs := logger.NewTesting("diagnostic-handler-test")
245-
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
243+
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)
246244

247245
mockDiagProvider.EXPECT().DiagnosticHooks().Return([]diagnostics.Hook{})
248246
mockDiagProvider.EXPECT().PerformDiagnostics(mock.Anything, mock.Anything).Return([]runtime.ComponentUnitDiagnostic{})
@@ -281,7 +279,7 @@ func TestDiagnosticHandlerCommitErrorWithLogs(t *testing.T) {
281279
mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
282280
mockUploader := mockhandlers.NewUploader(t)
283281
testLogger, observedLogs := logger.NewTesting("diagnostic-handler-test")
284-
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
282+
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)
285283

286284
mockDiagProvider.EXPECT().DiagnosticHooks().Return([]diagnostics.Hook{})
287285
mockDiagProvider.EXPECT().PerformDiagnostics(mock.Anything, mock.Anything).Return([]runtime.ComponentUnitDiagnostic{})
@@ -321,7 +319,7 @@ func TestDiagnosticHandlerContexteExpiredErrorWithLogs(t *testing.T) {
321319
mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
322320
mockUploader := mockhandlers.NewUploader(t)
323321
testLogger, observedLogs := logger.NewTesting("diagnostic-handler-test")
324-
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
322+
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)
325323

326324
mockDiagProvider.EXPECT().DiagnosticHooks().Return([]diagnostics.Hook{})
327325

@@ -365,7 +363,7 @@ func TestDiagnosticHandlerWithCPUProfile(t *testing.T) {
365363
mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
366364
mockUploader := mockhandlers.NewUploader(t)
367365
testLogger, _ := logger.NewTesting("diagnostic-handler-test")
368-
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
366+
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)
369367

370368
mockDiagProvider.EXPECT().DiagnosticHooks().Return([]diagnostics.Hook{hook1})
371369
mockDiagProvider.EXPECT().PerformDiagnostics(mock.Anything, mock.Anything).Return([]runtime.ComponentUnitDiagnostic{mockUnitDiagnostic})

‎internal/pkg/agent/application/application.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,8 @@ func New(
177177
EndpointSignedComponentModifier(),
178178
)
179179

180-
managed, err = newManagedConfigManager(ctx, log, agentInfo, cfg, store, runtime, fleetInitTimeout, upgrader)
180+
// TODO: stop using global state
181+
managed, err = newManagedConfigManager(ctx, log, agentInfo, cfg, store, runtime, fleetInitTimeout, paths.Top(), upgrader)
181182
if err != nil {
182183
return nil, nil, nil, err
183184
}

‎internal/pkg/agent/application/dispatcher/dispatcher.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,13 @@ type ActionDispatcher struct {
4444
queue priorityQueue
4545
rt *retryConfig
4646
errCh chan error
47+
topPath string
4748

4849
lastUpgradeDetails *details.Details
4950
}
5051

5152
// New creates a new action dispatcher.
52-
func New(log *logger.Logger, def actions.Handler, queue priorityQueue) (*ActionDispatcher, error) {
53+
func New(log *logger.Logger, topPath string, def actions.Handler, queue priorityQueue) (*ActionDispatcher, error) {
5354
var err error
5455
if log == nil {
5556
log, err = logger.New("action_dispatcher", false)
@@ -69,6 +70,7 @@ func New(log *logger.Logger, def actions.Handler, queue priorityQueue) (*ActionD
6970
queue: queue,
7071
rt: defaultRetryConfig(),
7172
errCh: make(chan error),
73+
topPath: topPath,
7274
}, nil
7375
}
7476

‎internal/pkg/agent/application/dispatcher/dispatcher_test.go

+17-17
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ func TestActionDispatcher(t *testing.T) {
121121
queue := &mockQueue{}
122122
queue.On("Save").Return(nil).Once()
123123
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
124-
d, err := New(nil, def, queue)
124+
d, err := New(nil, t.TempDir(), def, queue)
125125
require.NoError(t, err)
126126

127127
success1 := &mockHandler{}
@@ -163,7 +163,7 @@ func TestActionDispatcher(t *testing.T) {
163163
queue := &mockQueue{}
164164
queue.On("Save").Return(nil).Once()
165165
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
166-
d, err := New(nil, def, queue)
166+
d, err := New(nil, t.TempDir(), def, queue)
167167
require.NoError(t, err)
168168

169169
action := &mockOtherAction{}
@@ -187,7 +187,7 @@ func TestActionDispatcher(t *testing.T) {
187187

188188
def := &mockHandler{}
189189
queue := &mockQueue{}
190-
d, err := New(nil, def, queue)
190+
d, err := New(nil, t.TempDir(), def, queue)
191191
require.NoError(t, err)
192192

193193
err = d.Register(&mockAction{}, success1)
@@ -207,7 +207,7 @@ func TestActionDispatcher(t *testing.T) {
207207
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
208208
queue.On("Add", mock.Anything, mock.Anything).Once()
209209

210-
d, err := New(nil, def, queue)
210+
d, err := New(nil, t.TempDir(), def, queue)
211211
require.NoError(t, err)
212212
err = d.Register(&mockAction{}, def)
213213
require.NoError(t, err)
@@ -242,7 +242,7 @@ func TestActionDispatcher(t *testing.T) {
242242
queue.On("Save").Return(nil).Once()
243243
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
244244

245-
d, err := New(nil, def, queue)
245+
d, err := New(nil, t.TempDir(), def, queue)
246246
require.NoError(t, err)
247247
err = d.Register(&mockAction{}, def)
248248
require.NoError(t, err)
@@ -282,7 +282,7 @@ func TestActionDispatcher(t *testing.T) {
282282
queue.On("Save").Return(nil).Once()
283283
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{action1}).Once()
284284

285-
d, err := New(nil, def, queue)
285+
d, err := New(nil, t.TempDir(), def, queue)
286286
require.NoError(t, err)
287287
err = d.Register(&mockAction{}, def)
288288
require.NoError(t, err)
@@ -309,7 +309,7 @@ func TestActionDispatcher(t *testing.T) {
309309
queue.On("Save").Return(nil).Once()
310310
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
311311

312-
d, err := New(nil, def, queue)
312+
d, err := New(nil, t.TempDir(), def, queue)
313313
require.NoError(t, err)
314314
err = d.Register(&mockAction{}, def)
315315
require.NoError(t, err)
@@ -335,7 +335,7 @@ func TestActionDispatcher(t *testing.T) {
335335
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
336336
queue.On("Add", mock.Anything, mock.Anything).Once()
337337

338-
d, err := New(nil, def, queue)
338+
d, err := New(nil, t.TempDir(), def, queue)
339339
require.NoError(t, err)
340340
err = d.Register(&mockRetryableAction{}, def)
341341
require.NoError(t, err)
@@ -369,7 +369,7 @@ func TestActionDispatcher(t *testing.T) {
369369
queue.On("Save").Return(nil).Once()
370370
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
371371

372-
d, err := New(nil, def, queue)
372+
d, err := New(nil, t.TempDir(), def, queue)
373373
require.NoError(t, err)
374374
err = d.Register(&mockAction{}, def)
375375
require.NoError(t, err)
@@ -412,7 +412,7 @@ func TestActionDispatcher(t *testing.T) {
412412
queue.On("Save").Return(nil).Times(2)
413413
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Times(2)
414414

415-
d, err := New(nil, def, queue)
415+
d, err := New(nil, t.TempDir(), def, queue)
416416
require.NoError(t, err)
417417
err = d.Register(&mockAction{}, def)
418418
require.NoError(t, err)
@@ -464,7 +464,7 @@ func TestActionDispatcher(t *testing.T) {
464464
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
465465
queue.On("CancelType", mock.Anything).Return(1).Once()
466466

467-
d, err := New(nil, def, queue)
467+
d, err := New(nil, t.TempDir(), def, queue)
468468
require.NoError(t, err)
469469

470470
var gotDetails *details.Details
@@ -514,7 +514,7 @@ func TestActionDispatcher(t *testing.T) {
514514
Once()
515515
queue.On("CancelType", mock.Anything).Return(1).Once()
516516

517-
d, err := New(nil, def, queue)
517+
d, err := New(nil, t.TempDir(), def, queue)
518518
require.NoError(t, err)
519519

520520
var gotDetails *details.Details
@@ -556,7 +556,7 @@ func TestActionDispatcher(t *testing.T) {
556556
Once()
557557
queue.On("CancelType", mock.Anything).Return(1).Once()
558558

559-
d, err := New(nil, def, queue)
559+
d, err := New(nil, t.TempDir(), def, queue)
560560
require.NoError(t, err)
561561

562562
wantDetail := &details.Details{
@@ -598,7 +598,7 @@ func TestActionDispatcher(t *testing.T) {
598598
Once()
599599
queue.On("CancelType", mock.Anything).Return(1).Once()
600600

601-
d, err := New(nil, def, queue)
601+
d, err := New(nil, t.TempDir(), def, queue)
602602
require.NoError(t, err)
603603

604604
var gotDetails *details.Details
@@ -628,7 +628,7 @@ func Test_ActionDispatcher_scheduleRetry(t *testing.T) {
628628

629629
t.Run("no more attmpts", func(t *testing.T) {
630630
queue := &mockQueue{}
631-
d, err := New(nil, def, queue)
631+
d, err := New(nil, t.TempDir(), def, queue)
632632
require.NoError(t, err)
633633

634634
action := &mockRetryableAction{}
@@ -645,7 +645,7 @@ func Test_ActionDispatcher_scheduleRetry(t *testing.T) {
645645
queue := &mockQueue{}
646646
queue.On("Save").Return(nil).Once()
647647
queue.On("Add", mock.Anything, mock.Anything).Once()
648-
d, err := New(nil, def, queue)
648+
d, err := New(nil, t.TempDir(), def, queue)
649649
require.NoError(t, err)
650650

651651
action := &mockRetryableAction{}
@@ -734,7 +734,7 @@ func TestReportNextScheduledUpgrade(t *testing.T) {
734734
def := &mockHandler{}
735735

736736
queue := &mockQueue{}
737-
d, err := New(nil, def, queue)
737+
d, err := New(nil, t.TempDir(), def, queue)
738738
require.NoError(t, err, "could not create dispatcher")
739739

740740
for name, test := range cases {

‎internal/pkg/agent/application/managed_mode.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ func newManagedConfigManager(
6565
storeSaver storage.Store,
6666
runtime *runtime.Manager,
6767
fleetInitTimeout time.Duration,
68+
topPath string,
6869
clientSetters ...actions.ClientSetter,
6970
) (*managedConfigManager, error) {
7071
client, err := fleetclient.NewAuthWithConfig(log, cfg.Fleet.AccessAPIKey, cfg.Fleet.Client)
@@ -86,7 +87,7 @@ func newManagedConfigManager(
8687
return nil, fmt.Errorf("unable to initialize action queue: %w", err)
8788
}
8889

89-
actionDispatcher, err := dispatcher.New(log, handlers.NewDefault(log), actionQueue)
90+
actionDispatcher, err := dispatcher.New(log, topPath, handlers.NewDefault(log), actionQueue)
9091
if err != nil {
9192
return nil, fmt.Errorf("unable to initialize action dispatcher: %w", err)
9293
}
@@ -392,6 +393,7 @@ func (m *managedConfigManager) initDispatcher(canceller context.CancelFunc) *han
392393
&fleetapi.ActionDiagnostics{},
393394
handlers.NewDiagnostics(
394395
m.log,
396+
paths.Top(), // TODO: stop using global state
395397
m.coord,
396398
m.cfg.Settings.MonitoringConfig.Diagnostics.Limit,
397399
uploader.New(m.agentInfo.AgentID(), m.client, m.cfg.Settings.MonitoringConfig.Diagnostics.Uploader),

‎internal/pkg/agent/application/paths/common.go

+7-2
Original file line numberDiff line numberDiff line change
@@ -119,10 +119,15 @@ func TempDir() string {
119119

120120
// Home returns a directory where binary lives
121121
func Home() string {
122+
return HomeFrom(topPath)
123+
}
124+
125+
func HomeFrom(topDirPath string) string {
122126
if unversionedHome {
123-
return topPath
127+
return topDirPath
124128
}
125-
return VersionedHome(topPath)
129+
130+
return VersionedHome(topDirPath)
126131
}
127132

128133
// IsVersionHome returns true if the Home path is versioned based on build.

‎internal/pkg/agent/cmd/diagnostics.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616

1717
"github.com/spf13/cobra"
1818

19+
"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
1920
"github.com/elastic/elastic-agent/internal/pkg/cli"
2021
"github.com/elastic/elastic-agent/internal/pkg/diagnostics"
2122
)
@@ -68,7 +69,7 @@ func diagnosticCmd(streams *cli.IOStreams, cmd *cobra.Command) error {
6869
return fmt.Errorf("failed collecting diagnostics: %w", err)
6970
}
7071

71-
if err := diagnostics.ZipArchive(streams.Err, f, agentDiag, unitDiags, compDiags, excludeEvents); err != nil {
72+
if err := diagnostics.ZipArchive(streams.Err, f, paths.Top(), agentDiag, unitDiags, compDiags, excludeEvents); err != nil {
7273
return fmt.Errorf("unable to create archive %q: %w", filepath, err)
7374
}
7475
fmt.Fprintf(streams.Out, "Created diagnostics archive %q\n", filepath)

0 commit comments

Comments
 (0)