From 249873df5ff241e0eabd8bca33b539b70edcace0 Mon Sep 17 00:00:00 2001 From: Kent Rancourt Date: Tue, 3 Sep 2024 17:01:54 -0400 Subject: [PATCH] slight directive engine refactoring Signed-off-by: Kent Rancourt --- internal/directives/copy_directive.go | 7 ++-- internal/directives/directive.go | 34 ++++++++++++++---- internal/directives/engine.go | 23 ++++++++----- internal/directives/engine_test.go | 38 +++++++++++---------- internal/directives/git_clone_directive.go | 7 ++-- internal/directives/git_commit_directive.go | 7 ++-- internal/directives/git_push_directive.go | 7 ++-- 7 files changed, 79 insertions(+), 44 deletions(-) diff --git a/internal/directives/copy_directive.go b/internal/directives/copy_directive.go index 0d1aabf7b8..dac8baa411 100644 --- a/internal/directives/copy_directive.go +++ b/internal/directives/copy_directive.go @@ -39,15 +39,16 @@ func (d *copyDirective) Name() string { } func (d *copyDirective) Run(_ context.Context, stepCtx *StepContext) (Result, error) { + failure := Result{Status: StatusFailure} cfg, err := configToStruct[copyConfig](stepCtx.Config) if err != nil { - return ResultFailure, fmt.Errorf("could not convert config into copy config: %w", err) + return failure, fmt.Errorf("could not convert config into copy config: %w", err) } if err = cfg.Validate(); err != nil { - return ResultFailure, fmt.Errorf("invalid copy config: %w", err) + return failure, fmt.Errorf("invalid copy config: %w", err) } // TODO: add implementation here - return ResultSuccess, nil + return failure, nil } diff --git a/internal/directives/directive.go b/internal/directives/directive.go index 2d2c8588dd..c349088e72 100644 --- a/internal/directives/directive.go +++ b/internal/directives/directive.go @@ -90,6 +90,17 @@ func (s State) Get(key string) (any, bool) { return value, ok } +// DeepCopy returns a deep copy of the configuration. +func (s *State) DeepCopy() State { + if s == nil { + return nil + } + // TODO(hidde): we piggyback on the runtime package for now, as we expect + // the configuration to originate from a Kubernetes API object. We should + // consider writing our own implementation in the future. + return runtime.DeepCopyJSON(*s) +} + // Config is a map of configuration values that can be passed to a step. // The keys and values are arbitrary, and the step is responsible for // interpreting them. @@ -106,16 +117,27 @@ func (c Config) DeepCopy() Config { return runtime.DeepCopyJSON(c) } -// Result is a type that represents the result of a Directive. -type Result string +// Status is a type that represents the high-level outcome of a directive +// execution. +type Status string const ( - // ResultSuccess is the result of a successful directive. - ResultSuccess Result = "Success" - // ResultFailure is the result of a failed directive. - ResultFailure Result = "Failure" + // StatusSuccess is the result of a successful directive execution. + StatusSuccess Status = "Success" + // StatusFailure is the result of a failed directive execution. + StatusFailure Status = "Failure" ) +// Result represents the outcome of a directive execution, including its status +// (e.g. Success or Failure) and any output (State) that the execution engine +// executing the directive must append to the shared state. +type Result struct { + // Status is the high-level outcome of the directive execution. + Status Status + // Output is the output of the directive execution. + Output State +} + // Directive is an interface that a directive must implement. A directive is // a responsible for executing a specific action, and may modify the provided // context to allow subsequent directives to access the results of its diff --git a/internal/directives/engine.go b/internal/directives/engine.go index 8a591fd564..6b109570a9 100644 --- a/internal/directives/engine.go +++ b/internal/directives/engine.go @@ -45,11 +45,11 @@ func NewEngine( } // Execute runs the provided list of directives in sequence. -func (e *Engine) Execute(ctx context.Context, steps []Step) (Result, error) { +func (e *Engine) Execute(ctx context.Context, steps []Step) (Status, error) { // TODO(hidde): allow the workDir to be restored from a previous execution. workDir, err := os.MkdirTemp("", "run-") if err != nil { - return ResultFailure, fmt.Errorf("temporary working directory creation failed: %w", err) + return StatusFailure, fmt.Errorf("temporary working directory creation failed: %w", err) } defer os.RemoveAll(workDir) @@ -59,16 +59,18 @@ func (e *Engine) Execute(ctx context.Context, steps []Step) (Result, error) { for _, d := range steps { select { case <-ctx.Done(): - return ResultFailure, ctx.Err() + return StatusFailure, ctx.Err() default: reg, err := e.registry.GetDirectiveRegistration(d.Directive) if err != nil { - return ResultFailure, fmt.Errorf("failed to get step %q: %w", d.Directive, err) + return StatusFailure, fmt.Errorf("failed to get step %q: %w", d.Directive, err) } + stateCopy := state.DeepCopy() + stepCtx := &StepContext{ WorkDir: workDir, - SharedState: state, + SharedState: stateCopy, Alias: d.Alias, Config: d.Config.DeepCopy(), } @@ -83,10 +85,15 @@ func (e *Engine) Execute(ctx context.Context, steps []Step) (Result, error) { stepCtx.ArgoCDClient = e.argoCDClient } - if result, err := reg.Directive.Run(ctx, stepCtx); err != nil { - return result, fmt.Errorf("failed to run step %q: %w", d.Directive, err) + result, err := reg.Directive.Run(ctx, stepCtx) + if err != nil { + return result.Status, fmt.Errorf("failed to run step %q: %w", d.Directive, err) + } + + if d.Alias != "" { + state[d.Alias] = result.Output } } } - return ResultSuccess, nil + return StatusSuccess, nil } diff --git a/internal/directives/engine_test.go b/internal/directives/engine_test.go index 73c7d3cd04..6a4b41f9d6 100644 --- a/internal/directives/engine_test.go +++ b/internal/directives/engine_test.go @@ -10,12 +10,14 @@ import ( ) func TestEngine_Execute(t *testing.T) { + failureResult := Result{Status: StatusFailure} + successResult := Result{Status: StatusSuccess} tests := []struct { name string directives []Step initRegistry func() DirectiveRegistry ctx context.Context - assertions func(t *testing.T, result Result, err error) + assertions func(t *testing.T, status Status, err error) }{ { name: "success: single directive", @@ -27,15 +29,15 @@ func TestEngine_Execute(t *testing.T) { registry.RegisterDirective( &mockDirective{ name: "mock", - runResult: ResultSuccess, + runResult: successResult, }, nil, ) return registry }, ctx: context.Background(), - assertions: func(t *testing.T, result Result, err error) { - assert.Equal(t, ResultSuccess, result) + assertions: func(t *testing.T, status Status, err error) { + assert.Equal(t, StatusSuccess, status) assert.NoError(t, err) }, }, @@ -50,22 +52,22 @@ func TestEngine_Execute(t *testing.T) { registry.RegisterDirective( &mockDirective{ name: "mock1", - runResult: ResultSuccess, + runResult: successResult, }, nil, ) registry.RegisterDirective( &mockDirective{ name: "mock2", - runResult: ResultSuccess, + runResult: successResult, }, nil, ) return registry }, ctx: context.Background(), - assertions: func(t *testing.T, result Result, err error) { - assert.Equal(t, ResultSuccess, result) + assertions: func(t *testing.T, status Status, err error) { + assert.Equal(t, StatusSuccess, status) assert.NoError(t, err) }, }, @@ -78,8 +80,8 @@ func TestEngine_Execute(t *testing.T) { return make(DirectiveRegistry) }, ctx: context.Background(), - assertions: func(t *testing.T, result Result, err error) { - assert.Equal(t, ResultFailure, result) + assertions: func(t *testing.T, status Status, err error) { + assert.Equal(t, StatusFailure, status) assert.ErrorContains(t, err, "not found") }, }, @@ -93,7 +95,7 @@ func TestEngine_Execute(t *testing.T) { registry.RegisterDirective( &mockDirective{ name: "failing", - runResult: ResultFailure, + runResult: failureResult, runErr: errors.New("something went wrong"), }, nil, @@ -101,8 +103,8 @@ func TestEngine_Execute(t *testing.T) { return registry }, ctx: context.Background(), - assertions: func(t *testing.T, result Result, err error) { - assert.Equal(t, ResultFailure, result) + assertions: func(t *testing.T, status Status, err error) { + assert.Equal(t, StatusFailure, status) assert.ErrorContains(t, err, "something went wrong") }, }, @@ -119,7 +121,7 @@ func TestEngine_Execute(t *testing.T) { name: "mock", runFunc: func(ctx context.Context, _ *StepContext) (Result, error) { <-ctx.Done() // Wait for context to be canceled - return ResultSuccess, nil + return successResult, nil }, }, nil, @@ -134,8 +136,8 @@ func TestEngine_Execute(t *testing.T) { }() return ctx }(), - assertions: func(t *testing.T, result Result, err error) { - assert.Equal(t, ResultFailure, result) + assertions: func(t *testing.T, status Status, err error) { + assert.Equal(t, StatusFailure, status) assert.ErrorIs(t, err, context.Canceled) }, }, @@ -143,8 +145,8 @@ func TestEngine_Execute(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { engine := NewEngine(tt.initRegistry(), nil, nil, nil) - result, err := engine.Execute(tt.ctx, tt.directives) - tt.assertions(t, result, err) + status, err := engine.Execute(tt.ctx, tt.directives) + tt.assertions(t, status, err) }) } } diff --git a/internal/directives/git_clone_directive.go b/internal/directives/git_clone_directive.go index 8f105b44cf..b8d29a9190 100644 --- a/internal/directives/git_clone_directive.go +++ b/internal/directives/git_clone_directive.go @@ -41,18 +41,19 @@ func (g *gitCloneDirective) Run( _ context.Context, stepCtx *StepContext, ) (Result, error) { + failure := Result{Status: StatusFailure} // Validate the configuration against the JSON Schema if err := validate( g.schemaLoader, gojsonschema.NewGoLoader(stepCtx.Config), "git-clone", ); err != nil { - return ResultFailure, err + return failure, err } if _, err := configToStruct[GitCloneConfig](stepCtx.Config); err != nil { - return ResultFailure, + return failure, fmt.Errorf("could not convert config into git-clone config: %w", err) } // TODO: Add implementation here - return ResultSuccess, nil + return Result{Status: StatusSuccess}, nil } diff --git a/internal/directives/git_commit_directive.go b/internal/directives/git_commit_directive.go index c1d44f8c91..e4b7d2f7da 100644 --- a/internal/directives/git_commit_directive.go +++ b/internal/directives/git_commit_directive.go @@ -35,18 +35,19 @@ func (g *gitCommitDirective) Run( _ context.Context, stepCtx *StepContext, ) (Result, error) { + failure := Result{Status: StatusFailure} // Validate the configuration against the JSON Schema if err := validate( g.schemaLoader, gojsonschema.NewGoLoader(stepCtx.Config), "git-commit", ); err != nil { - return ResultFailure, err + return failure, err } if _, err := configToStruct[GitCommitConfig](stepCtx.Config); err != nil { - return ResultFailure, + return failure, fmt.Errorf("could not convert config into git-commit config: %w", err) } // TODO: Add implementation here - return ResultSuccess, nil + return Result{Status: StatusSuccess}, nil } diff --git a/internal/directives/git_push_directive.go b/internal/directives/git_push_directive.go index 068698285d..1115fed91d 100644 --- a/internal/directives/git_push_directive.go +++ b/internal/directives/git_push_directive.go @@ -35,18 +35,19 @@ func (g *gitPushDirective) Run( _ context.Context, stepCtx *StepContext, ) (Result, error) { + failure := Result{Status: StatusFailure} // Validate the configuration against the JSON Schema if err := validate( g.schemaLoader, gojsonschema.NewGoLoader(stepCtx.Config), "git-push", ); err != nil { - return ResultFailure, err + return failure, err } if _, err := configToStruct[GitPushConfig](stepCtx.Config); err != nil { - return ResultFailure, + return failure, fmt.Errorf("could not convert config into git-push config: %w", err) } // TODO: Add implementation here - return ResultSuccess, nil + return Result{Status: StatusSuccess}, nil }