Skip to content

Commit

Permalink
Stop setting global state on tests
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
belimawr committed Jun 5, 2024
1 parent 24de2d0 commit 4b8b8e0
Show file tree
Hide file tree
Showing 10 changed files with 76 additions and 47 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/elastic/elastic-agent/pkg/control/v2/client"
"github.com/elastic/elastic-agent/pkg/control/v2/cproto"

"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
"github.com/elastic/elastic-agent/internal/pkg/core/monitoring/config"
"github.com/elastic/elastic-agent/internal/pkg/diagnostics"
"github.com/elastic/elastic-agent/internal/pkg/fleetapi"
Expand Down Expand Up @@ -67,15 +68,20 @@ type Diagnostics struct {
diagProvider diagnosticsProvider
limiter *rate.Limiter
uploader Uploader
topPath string
}

// NewDiagnostics returns a new Diagnostics handler.
func NewDiagnostics(log abstractLogger, coord diagnosticsProvider, cfg config.Limit, uploader Uploader) *Diagnostics {
func NewDiagnostics(log abstractLogger, topPath string, coord diagnosticsProvider, cfg config.Limit, uploader Uploader) *Diagnostics {
if topPath == "" {
topPath = paths.Top()
}
return &Diagnostics{
log: log,
diagProvider: coord,
limiter: rate.NewLimiter(rate.Every(cfg.Interval), cfg.Burst),
uploader: uploader,
topPath: topPath,
}
}

Expand Down Expand Up @@ -155,7 +161,7 @@ func (h *Diagnostics) collectDiag(ctx context.Context, action *fleetapi.ActionDi
h.log.Warn(str)
}
}()
err := diagnostics.ZipArchive(&wBuf, &b, aDiag, uDiag, cDiag, action.ExcludeEventsLog)
err := diagnostics.ZipArchive(&wBuf, &b, h.topPath, aDiag, uDiag, cDiag, action.ExcludeEventsLog)
if err != nil {
h.log.Errorw(
"diagnostics action handler failed generate zip archive",
Expand Down Expand Up @@ -344,7 +350,7 @@ func (h *Diagnostics) diagFile(
h.log.Warn(str)
}
}()
if err := diagnostics.ZipArchive(&wBuf, f, aDiag, uDiag, cDiag, excludeEvents); err != nil {
if err := diagnostics.ZipArchive(&wBuf, f, h.topPath, aDiag, uDiag, cDiag, excludeEvents); err != nil {
os.Remove(name)
return nil, 0, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,14 @@ var (
)

func TestDiagnosticHandlerHappyPathWithLogs(t *testing.T) {

tempAgentRoot := t.TempDir()
paths.SetTop(tempAgentRoot)
err := os.MkdirAll(path.Join(tempAgentRoot, "data"), 0755)
require.NoError(t, err)

mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
mockUploader := mockhandlers.NewUploader(t)
testLogger, observedLogs := logger.NewTesting("diagnostic-handler-test")
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)

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

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

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

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

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

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

Expand Down Expand Up @@ -365,7 +363,7 @@ func TestDiagnosticHandlerWithCPUProfile(t *testing.T) {
mockDiagProvider := mockhandlers.NewDiagnosticsProvider(t)
mockUploader := mockhandlers.NewUploader(t)
testLogger, _ := logger.NewTesting("diagnostic-handler-test")
handler := NewDiagnostics(testLogger, mockDiagProvider, defaultRateLimit, mockUploader)
handler := NewDiagnostics(testLogger, tempAgentRoot, mockDiagProvider, defaultRateLimit, mockUploader)

mockDiagProvider.EXPECT().DiagnosticHooks().Return([]diagnostics.Hook{hook1})
mockDiagProvider.EXPECT().PerformDiagnostics(mock.Anything, mock.Anything).Return([]runtime.ComponentUnitDiagnostic{mockUnitDiagnostic})
Expand Down
3 changes: 2 additions & 1 deletion internal/pkg/agent/application/application.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ func New(
EndpointSignedComponentModifier(),
)

managed, err = newManagedConfigManager(ctx, log, agentInfo, cfg, store, runtime, fleetInitTimeout, upgrader)
// TODO: stop using global state
managed, err = newManagedConfigManager(ctx, log, agentInfo, cfg, store, runtime, fleetInitTimeout, paths.Top(), upgrader)
if err != nil {
return nil, nil, nil, err
}
Expand Down
4 changes: 3 additions & 1 deletion internal/pkg/agent/application/dispatcher/dispatcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,13 @@ type ActionDispatcher struct {
queue priorityQueue
rt *retryConfig
errCh chan error
topPath string

lastUpgradeDetails *details.Details
}

// New creates a new action dispatcher.
func New(log *logger.Logger, def actions.Handler, queue priorityQueue) (*ActionDispatcher, error) {
func New(log *logger.Logger, topPath string, def actions.Handler, queue priorityQueue) (*ActionDispatcher, error) {
var err error
if log == nil {
log, err = logger.New("action_dispatcher", false)
Expand All @@ -69,6 +70,7 @@ func New(log *logger.Logger, def actions.Handler, queue priorityQueue) (*ActionD
queue: queue,
rt: defaultRetryConfig(),
errCh: make(chan error),
topPath: topPath,
}, nil
}

Expand Down
34 changes: 17 additions & 17 deletions internal/pkg/agent/application/dispatcher/dispatcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func TestActionDispatcher(t *testing.T) {
queue := &mockQueue{}
queue.On("Save").Return(nil).Once()
queue.On("DequeueActions").Return([]fleetapi.ScheduledAction{}).Once()
d, err := New(nil, def, queue)
d, err := New(nil, t.TempDir(), def, queue)
require.NoError(t, err)

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

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

def := &mockHandler{}
queue := &mockQueue{}
d, err := New(nil, def, queue)
d, err := New(nil, t.TempDir(), def, queue)
require.NoError(t, err)

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

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

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

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

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

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

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

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

d, err := New(nil, def, queue)
d, err := New(nil, t.TempDir(), def, queue)
require.NoError(t, err)

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

d, err := New(nil, def, queue)
d, err := New(nil, t.TempDir(), def, queue)
require.NoError(t, err)

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

d, err := New(nil, def, queue)
d, err := New(nil, t.TempDir(), def, queue)
require.NoError(t, err)

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

d, err := New(nil, def, queue)
d, err := New(nil, t.TempDir(), def, queue)
require.NoError(t, err)

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

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

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

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

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

for name, test := range cases {
Expand Down
4 changes: 3 additions & 1 deletion internal/pkg/agent/application/managed_mode.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ func newManagedConfigManager(
storeSaver storage.Store,
runtime *runtime.Manager,
fleetInitTimeout time.Duration,
topPath string,
clientSetters ...actions.ClientSetter,
) (*managedConfigManager, error) {
client, err := fleetclient.NewAuthWithConfig(log, cfg.Fleet.AccessAPIKey, cfg.Fleet.Client)
Expand All @@ -86,7 +87,7 @@ func newManagedConfigManager(
return nil, fmt.Errorf("unable to initialize action queue: %w", err)
}

actionDispatcher, err := dispatcher.New(log, handlers.NewDefault(log), actionQueue)
actionDispatcher, err := dispatcher.New(log, topPath, handlers.NewDefault(log), actionQueue)
if err != nil {
return nil, fmt.Errorf("unable to initialize action dispatcher: %w", err)
}
Expand Down Expand Up @@ -392,6 +393,7 @@ func (m *managedConfigManager) initDispatcher(canceller context.CancelFunc) *han
&fleetapi.ActionDiagnostics{},
handlers.NewDiagnostics(
m.log,
paths.Top(), // TODO: stop using global state
m.coord,
m.cfg.Settings.MonitoringConfig.Diagnostics.Limit,
uploader.New(m.agentInfo.AgentID(), m.client, m.cfg.Settings.MonitoringConfig.Diagnostics.Uploader),
Expand Down
9 changes: 7 additions & 2 deletions internal/pkg/agent/application/paths/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,15 @@ func TempDir() string {

// Home returns a directory where binary lives
func Home() string {
return HomeFrom(topPath)
}

func HomeFrom(topDirPath string) string {
if unversionedHome {
return topPath
return topDirPath
}
return VersionedHome(topPath)

return VersionedHome(topDirPath)
}

// IsVersionHome returns true if the Home path is versioned based on build.
Expand Down
3 changes: 2 additions & 1 deletion internal/pkg/agent/cmd/diagnostics.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

"github.com/spf13/cobra"

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

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

0 comments on commit 4b8b8e0

Please sign in to comment.