From 42aed991ac3d313b1d18e8f48cfc9e19b75e0bb3 Mon Sep 17 00:00:00 2001 From: Dasha Komsa Date: Sat, 9 Mar 2024 00:21:12 +0000 Subject: [PATCH] feedback fixes Signed-off-by: Dasha Komsa --- internal/ansible/ansible.go | 31 ++++++++++++++++++------------- internal/ansible/ansible_test.go | 12 +++++++++--- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/internal/ansible/ansible.go b/internal/ansible/ansible.go index 653190f..87dd04a 100644 --- a/internal/ansible/ansible.go +++ b/internal/ansible/ansible.go @@ -56,6 +56,9 @@ const ( errMkdir = "cannot make directory" ) +// using a variable for uuid generator allows for stubbing in tests +var generateUUID = uuid.New + const ( // AnnotationKeyPolicyRun is the name of an annotation which instructs // the provider how to run the corresponding Ansible contents @@ -333,7 +336,6 @@ type Runner struct { behaviorVars map[string]string cmdFunc cmdFuncType // returns a Cmd that runs ansible-runner workDir string - AnsibleEnvDir string checkMode bool AnsibleRunPolicy *RunPolicy artifactsHistoryLimit int @@ -370,7 +372,7 @@ func (r *Runner) Run(ctx context.Context) (io.Reader, error) { dc := r.cmdFunc(r.behaviorVars, r.checkMode) dc.Args = append(dc.Args, "--rotate-artifacts", strconv.Itoa(r.artifactsHistoryLimit)) - id := uuid.New().String() + id := generateUUID().String() dc.Args = append(dc.Args, "--ident", id) if !r.checkMode { @@ -402,9 +404,10 @@ func (r *Runner) Run(ctx context.Context) (io.Reader, error) { if err := dc.Wait(); err != nil { jobEventsDir := filepath.Clean(filepath.Join(r.workDir, "artifacts", id, "job_events")) - failureReason, reasonErr := extractFailureReason(jobEventsDir) + failureReason, reasonErr := extractFailureReason(ctx, jobEventsDir) if reasonErr != nil { - log.FromContext(ctx).Error(err, "extracting ansible failure message") + log.FromContext(ctx).V(1).Info("extracting ansible failure message", "err", reasonErr) + return nil, err } return nil, fmt.Errorf("%w: %s", err, failureReason) @@ -413,8 +416,8 @@ func (r *Runner) Run(ctx context.Context) (io.Reader, error) { return &stdoutBuf, nil } -func extractFailureReason(eventsDir string) (string, error) { - evts, err := parseEvents(eventsDir) +func extractFailureReason(ctx context.Context, eventsDir string) (string, error) { + evts, err := parseEvents(ctx, eventsDir) if err != nil { return "", fmt.Errorf("parsing job events: %w", err) } @@ -441,24 +444,26 @@ func extractFailureReason(eventsDir string) (string, error) { return strings.Join(msgs, "; "), nil } -func parseEvents(dir string) ([]jobEvent, error) { +func parseEvents(ctx context.Context, dir string) ([]jobEvent, error) { files, err := os.ReadDir(dir) if err != nil { - return nil, fmt.Errorf("reading job events directory: %w", err) + return nil, fmt.Errorf("reading job events directory %q: %w", dir, err) } - evts := make([]jobEvent, len(files)) - for i, file := range files { + var evts []jobEvent + for _, file := range files { evtBytes, err := os.ReadFile(filepath.Clean(filepath.Join(dir, file.Name()))) if err != nil { - return nil, fmt.Errorf("reading job event file %q: %w", file.Name(), err) + log.FromContext(ctx).V(1).Info("reading job event file", "filename", file.Name(), "err", err) + continue } var evt jobEvent if err := json.Unmarshal(evtBytes, &evt); err != nil { - return nil, fmt.Errorf("unmarshaling job event from file %q: %w", file.Name(), err) + log.FromContext(ctx).V(1).Info("unmarshaling job event from file", "filename", file.Name(), "err", err) + continue } - evts[i] = evt + evts = append(evts, evt) } return evts, nil diff --git a/internal/ansible/ansible_test.go b/internal/ansible/ansible_test.go index dda4fd4..5b9ec91 100644 --- a/internal/ansible/ansible_test.go +++ b/internal/ansible/ansible_test.go @@ -29,6 +29,7 @@ import ( "github.com/crossplane-contrib/provider-ansible/apis/v1alpha1" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/google/uuid" "gotest.tools/v3/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -156,7 +157,7 @@ func TestInit(t *testing.T) { expectedRunner := &Runner{ Path: dir, cmdFunc: params.playbookCmdFunc(context.Background(), "playbook.yml", dir), - AnsibleEnvDir: filepath.Join(dir, "env"), + workDir: dir, AnsibleRunPolicy: &RunPolicy{"ObserveAndDelete"}, artifactsHistoryLimit: 3, } @@ -176,6 +177,9 @@ func TestInit(t *testing.T) { if runner.checkMode != expectedRunner.checkMode { t.Errorf("Unexpected Runner.checkMode %v expected %v", runner.checkMode, expectedRunner.checkMode) } + if runner.workDir != expectedRunner.workDir { + t.Errorf("Unexpected Runner.workDir %v expected %v", runner.workDir, expectedRunner.workDir) + } expectedCmd := expectedRunner.cmdFunc(nil, false) cmd := runner.cmdFunc(nil, false) @@ -194,12 +198,14 @@ func TestRun(t *testing.T) { // therefore checking its output also checks the args passed to it are correct return exec.CommandContext(context.Background(), "echo") }, - AnsibleEnvDir: filepath.Join(dir, "env"), AnsibleRunPolicy: &RunPolicy{"ObserveAndDelete"}, artifactsHistoryLimit: 3, } - expectedArgs := []string{"--rotate-artifacts", "3"} + expectedID := "217b3830-68fa-461b-90d1-1fb87c685010" + expectedArgs := []string{"--rotate-artifacts", "3", "--ident", expectedID} + + generateUUID = func() uuid.UUID { return uuid.MustParse(expectedID) } testCases := map[string]struct { checkMode bool