diff --git a/pkg/devcontainer/setup/lifecyclehooks.go b/pkg/devcontainer/setup/lifecyclehooks.go index 60bda082d..7568e6f28 100644 --- a/pkg/devcontainer/setup/lifecyclehooks.go +++ b/pkg/devcontainer/setup/lifecyclehooks.go @@ -52,6 +52,20 @@ func validWaitForPhase(phase LifecyclePhase) bool { return phase == PhaseInitializeCommand || slices.Contains(phaseOrder, phase) } +// phaseHasCommands reports whether any phaseHook in all matches +// the given phase and carries at least one command (or a runFunc). +func phaseHasCommands(all []phaseHook, phase LifecyclePhase) bool { + for _, ph := range all { + if ph.phase != phase { + continue + } + if ph.runFunc != nil || len(ph.params.commands) > 0 { + return true + } + } + return false +} + // resolveWaitFor normalises the raw waitFor string from the config, // falling back to the spec default for empty or invalid values. func resolveWaitFor(raw string) LifecyclePhase { @@ -182,6 +196,12 @@ func RunPreAttachHooks( } waitFor := resolveWaitFor(setupInfo.MergedConfig.WaitFor) + if !phaseHasCommands(all, waitFor) { + log.Debugf( + "waitFor phase %q has no commands configured; the split point is a no-op", + waitFor, + ) + } deferred, err := runWithWaitFor(all, waitFor) return DeferredHooks{hooks: deferred}, err } diff --git a/pkg/devcontainer/setup/lifecyclehooks_test.go b/pkg/devcontainer/setup/lifecyclehooks_test.go index d6b1a9e4c..c6dee9bdd 100644 --- a/pkg/devcontainer/setup/lifecyclehooks_test.go +++ b/pkg/devcontainer/setup/lifecyclehooks_test.go @@ -11,9 +11,11 @@ import ( "time" "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" + "go.uber.org/zap/zapcore" ) type LifecycleHookTestSuite struct { @@ -540,6 +542,58 @@ func (s *LifecycleHookTestSuite) TestDeferredHooksNotEmptyWithDotfiles() { assert.False(t, d.Empty(), "should not be empty when dotfiles runFunc is set") } +func (s *LifecycleHookTestSuite) TestPhaseHasCommandsTrue() { + all := []phaseHook{ + { + phase: PhaseOnCreate, + params: hookRunParams{ + commands: []types.LifecycleHook{{"": {"echo", "hi"}}}, + }, + }, + {phase: PhaseUpdateContent, params: hookRunParams{}}, + } + assert.True(s.T(), phaseHasCommands(all, PhaseOnCreate)) +} + +func (s *LifecycleHookTestSuite) TestPhaseHasCommandsFalseEmpty() { + all := makeTestPhaseHooks() // all phases have zero commands + assert.False(s.T(), phaseHasCommands(all, PhaseOnCreate)) +} + +func (s *LifecycleHookTestSuite) TestPhaseHasCommandsTrueRunFunc() { + all := []phaseHook{ + {phase: PhaseOnCreate, runFunc: func() error { return nil }}, + } + assert.True(s.T(), phaseHasCommands(all, PhaseOnCreate)) +} + +func (s *LifecycleHookTestSuite) TestWaitForEmptyPhaseLogsWarning() { + t := s.T() + logs := log.InitTestObserved(t, zapcore.DebugLevel) + + // All phases have no commands — waitFor triggers a debug warning. + all := makeTestPhaseHooks() + _, err := runWithWaitFor(all, DefaultWaitFor) + assert.NoError(t, err) + + // Reproduce the check from RunPreAttachHooks (which requires + // full container context we cannot set up in a unit test). + if !phaseHasCommands(all, DefaultWaitFor) { + log.Debugf( + "waitFor phase %q has no commands configured; the split point is a no-op", + DefaultWaitFor, + ) + } + + entries := logs.FilterMessage( + fmt.Sprintf( + "waitFor phase %q has no commands configured; the split point is a no-op", + DefaultWaitFor, + ), + ) + assert.Equal(t, 1, entries.Len(), "expected one debug warning about empty waitFor phase") +} + func TestLifecycleHookTestSuite(t *testing.T) { suite.Run(t, new(LifecycleHookTestSuite)) } diff --git a/pkg/log/testing.go b/pkg/log/testing.go index d66c005d9..7e6db1f62 100644 --- a/pkg/log/testing.go +++ b/pkg/log/testing.go @@ -3,7 +3,10 @@ package log import ( "testing" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" "go.uber.org/zap/zaptest" + "go.uber.org/zap/zaptest/observer" ) // InitTest replaces the package-level logger with a test logger. @@ -15,3 +18,15 @@ func InitTest(t testing.TB) { sugar = logger.Sugar() t.Cleanup(func() { sugar = prev }) } + +// InitTestObserved replaces the package-level logger with an observed +// logger that records all entries at the given level and above. +// Returns the observer so callers can assert on logged messages. +func InitTestObserved(t testing.TB, level zapcore.Level) *observer.ObservedLogs { + t.Helper() + prev := sugar + core, logs := observer.New(level) + sugar = zap.New(core).Sugar() + t.Cleanup(func() { sugar = prev }) + return logs +}