feedback fixes

Signed-off-by: Dasha Komsa <[email protected]>

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

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