fix: reduce galaxy requirements check noise and de-duplicate path checks

db_lib/AnsibleApp.go

@@ -48,6 +48,18 @@ func writeMD5Hash(requirementsFile string, requirementsHashFile string) error {
 	return os.WriteFile(requirementsHashFile, []byte(newFileMD5Hash), 0o644)
 }
 
+// fileExists reports whether the path exists and is a regular file (not a directory).
+func fileExists(p string) bool {
+	info, err := os.Stat(p)
+	return err == nil && !info.IsDir()
+}
+
+// dirExists reports whether the path exists and is a directory.
+func dirExists(p string) bool {
+	info, err := os.Stat(p)
+	return err == nil && info.IsDir()
+}
+
 type AnsibleApp struct {
 	Logger     task_logger.Logger
 	Playbook   *AnsiblePlaybook
@@ -75,10 +87,12 @@ func (t *AnsibleApp) Clear() {
 }
 
 func (t *AnsibleApp) InstallRequirements(args LocalAppInstallingArgs) error {
-	if err := t.installCollectionsRequirements(args.EnvironmentVars); err != nil {
+	rolePaths, collectionPaths := t.resolveGalaxyRequirements()
+
+	if err := t.installCollectionsRequirements(collectionPaths, args.EnvironmentVars); err != nil {
 		return err
 	}
-	if err := t.installRolesRequirements(args.EnvironmentVars); err != nil {
+	if err := t.installRolesRequirements(rolePaths, args.EnvironmentVars); err != nil {
 		return err
 	}
 	return nil
@@ -98,21 +112,19 @@ func (t *AnsibleApp) requirementsHashFilePath(requirementsType GalaxyRequirement
 	return path.Join(internalDir, fmt.Sprintf("requirements_%x_%s.md5", sum, requirementsType))
 }
 
+// installGalaxyRequirementsFile installs a single requirements file. The file is assumed to exist:
+// existence and logging of missing files is handled by resolveGalaxyRequirements. Installation is
+// skipped when the file content has not changed since the last successful install.
 func (t *AnsibleApp) installGalaxyRequirementsFile(requirementsType GalaxyRequirementsType, requirementsFilePath string, environmentVars []string) error {
 	requirementsHashFilePath := t.requirementsHashFilePath(requirementsType, requirementsFilePath)
 
-	if _, err := os.Stat(requirementsFilePath); err != nil {
-		t.Log("No " + requirementsFilePath + " file found. Skip galaxy install process.\n")
-		return nil
-	}
-
 	if hasRequirementsChanges(requirementsFilePath, requirementsHashFilePath) {
 		if err := t.runGalaxy([]string{
 			string(requirementsType),
-			"install",
-			"-r",
-			requirementsFilePath,
-			"--force",
+				      "install",
+				      "-r",
+				      requirementsFilePath,
+				      "--force",
 		}, environmentVars); err != nil {
 			return err
 		}
@@ -142,44 +154,101 @@ const (
 	GalaxyCollection GalaxyRequirementsType = "collection"
 )
 
-func (t *AnsibleApp) installRolesRequirements(environmentVars []string) (err error) {
-	// default roles path
-	err = t.installGalaxyRequirementsFile(GalaxyRole, path.Join(t.GetPlaybookDir(), "roles", "requirements.yml"), environmentVars)
-	if err != nil {
-		return
+// resolveGalaxyRequirements collects the requirements files that should be installed and returns
+// the existing paths split by type (roles, collections).
+//
+// Search rules:
+//   - <dir>/roles/requirements.yml and <dir>/collections/requirements.yml are type-specific
+//     subdirectory paths. If the subdirectory does not exist, the path is skipped silently.
+//     If the subdirectory exists but contains no requirements.yml, a warning is logged.
+//   - <dir>/requirements.yml is a shared file that may contain both roles and collections, so it
+//     is offered to both types. If none of the shared files exist anywhere, a single message
+//     listing the searched paths is logged.
+//
+// <dir> is checked both as the playbook directory and as the repository root.
+func (t *AnsibleApp) resolveGalaxyRequirements() (rolePaths []string, collectionPaths []string) {
+	playbookDir := t.GetPlaybookDir()
+	repoPath := t.getRepoPath()
+
+	// Base directories to search, de-duplicated (playbook dir may equal repo root).
+	baseDirs := []string{playbookDir}
+	if repoPath != playbookDir {
+		baseDirs = append(baseDirs, repoPath)
 	}
-	err = t.installGalaxyRequirementsFile(GalaxyRole, path.Join(t.GetPlaybookDir(), "requirements.yml"), environmentVars)
-	if err != nil {
-		return
+
+	// --- Type-specific subdirectory requirements: <dir>/roles|collections/requirements.yml ---
+	type subdir struct {
+		reqType GalaxyRequirementsType
+		dirName string
+		target  *[]string
+	}
+	subdirs := []subdir{
+		{GalaxyRole, "roles", &rolePaths},
+		{GalaxyCollection, "collections", &collectionPaths},
 	}
 
-	// alternative roles path
-	err = t.installGalaxyRequirementsFile(GalaxyRole, path.Join(t.getRepoPath(), "roles", "requirements.yml"), environmentVars)
-	if err != nil {
-		return
+	for _, base := range baseDirs {
+		for _, sd := range subdirs {
+			dir := path.Join(base, sd.dirName)
+			if !dirExists(dir) {
+				// No roles/ or collections/ directory: nothing to install, stay silent.
+				continue
+			}
+			reqFile := path.Join(dir, "requirements.yml")
+			if fileExists(reqFile) {
+				*sd.target = append(*sd.target, reqFile)
+			} else {
+				// Directory exists but has no requirements.yml: worth highlighting.
+				t.Log("Warning: " + dir + " exists but contains no requirements.yml.\n")
+			}
+		}
 	}
-	err = t.installGalaxyRequirementsFile(GalaxyRole, path.Join(t.getRepoPath(), "requirements.yml"), environmentVars)
+
+	// --- Shared requirements: <dir>/requirements.yml (may hold roles and collections) ---
+	var sharedSearched []string
+	var sharedFound []string
+	for _, base := range baseDirs {
+		reqFile := path.Join(base, "requirements.yml")
+		sharedSearched = append(sharedSearched, reqFile)
+		if fileExists(reqFile) {
+			sharedFound = append(sharedFound, reqFile)
+		}
+	}
+
+	if len(sharedFound) == 0 {
+		// None of the shared requirements files exist: log once, listing where we looked.
+		msg := "No requirements.yml found. Skip galaxy install process. Searched:"
+		for _, p := range sharedSearched {
+			msg += "\n  " + p
+		}
+		t.Log(msg + "\n")
+	} else {
+		// A shared file may contain both roles and collections, so offer it to both types.
+		for _, p := range sharedFound {
+			rolePaths = append(rolePaths, p)
+			collectionPaths = append(collectionPaths, p)
+		}
+	}
+
 	return
 }
 
-func (t *AnsibleApp) installCollectionsRequirements(environmentVars []string) (err error) {
-	// default collections path
-	err = t.installGalaxyRequirementsFile(GalaxyCollection, path.Join(t.GetPlaybookDir(), "collections", "requirements.yml"), environmentVars)
-	if err != nil {
-		return
-	}
-	err = t.installGalaxyRequirementsFile(GalaxyCollection, path.Join(t.GetPlaybookDir(), "requirements.yml"), environmentVars)
-	if err != nil {
-		return
+func (t *AnsibleApp) installRolesRequirements(paths []string, environmentVars []string) error {
+	for _, p := range paths {
+		if err := t.installGalaxyRequirementsFile(GalaxyRole, p, environmentVars); err != nil {
+			return err
+		}
 	}
+	return nil
+}
 
-	// alternative collections path
-	err = t.installGalaxyRequirementsFile(GalaxyCollection, path.Join(t.getRepoPath(), "collections", "requirements.yml"), environmentVars)
-	if err != nil {
-		return
+func (t *AnsibleApp) installCollectionsRequirements(paths []string, environmentVars []string) error {
+	for _, p := range paths {
+		if err := t.installGalaxyRequirementsFile(GalaxyCollection, p, environmentVars); err != nil {
+			return err
+		}
 	}
-	err = t.installGalaxyRequirementsFile(GalaxyCollection, path.Join(t.getRepoPath(), "requirements.yml"), environmentVars)
-	return
+	return nil
 }
 
 func (t *AnsibleApp) runGalaxy(args []string, environmentVars []string) error {

db_lib/AnsibleApp_test.go

@@ -0,0 +1,188 @@
+package db_lib
+
+import (
+	"os"
+	"os/exec"
+	"path"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/semaphoreui/semaphore/db"
+	"github.com/semaphoreui/semaphore/pkg/task_logger"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// collectingLogger is a minimal task_logger.Logger implementation that records
+// the messages passed to Log, so tests can assert on what was logged.
+type collectingLogger struct {
+	messages []string
+}
+
+func (l *collectingLogger) Log(msg string) { l.messages = append(l.messages, msg) }
+
+// The remaining methods satisfy the task_logger.Logger interface with no-ops.
+func (l *collectingLogger) Logf(format string, a ...any)                        {}
+func (l *collectingLogger) LogWithTime(now time.Time, msg string)               {}
+func (l *collectingLogger) LogfWithTime(now time.Time, format string, a ...any) {}
+func (l *collectingLogger) LogCmd(cmd *exec.Cmd)                                {}
+func (l *collectingLogger) SetStatus(status task_logger.TaskStatus)             {}
+func (l *collectingLogger) AddStatusListener(s task_logger.StatusListener)      {}
+func (l *collectingLogger) AddLogListener(s task_logger.LogListener)            {}
+func (l *collectingLogger) SetCommit(hash, message string)                      {}
+func (l *collectingLogger) WaitLog()                                            {}
+
+func (l *collectingLogger) joined() string {
+	return strings.Join(l.messages, "")
+}
+
+func (l *collectingLogger) countContaining(substr string) int {
+	n := 0
+	for _, m := range l.messages {
+		if strings.Contains(m, substr) {
+			n++
+		}
+	}
+	return n
+}
+
+// newTestApp builds an AnsibleApp whose repository points at dir (a local
+// repository, since the path is absolute) with the playbook at the repo root,
+// so GetPlaybookDir() == getRepoPath() == dir.
+func newTestApp(dir string, logger task_logger.Logger) *AnsibleApp {
+	return &AnsibleApp{
+		Logger: logger,
+		Template: db.Template{
+			ID:       1,
+			Playbook: "", // playbook at repo root
+		},
+		Repository: db.Repository{
+			ID:        1,
+			ProjectID: 1,
+			GitURL:    dir, // absolute path => RepositoryLocal
+		},
+	}
+}
+
+func mustMkdir(t *testing.T, p string) {
+	t.Helper()
+	require.NoError(t, os.MkdirAll(p, 0o755))
+}
+
+func mustWrite(t *testing.T, p string) {
+	t.Helper()
+	require.NoError(t, os.MkdirAll(path.Dir(p), 0o755))
+	require.NoError(t, os.WriteFile(p, []byte("---\n"), 0o644))
+}
+
+func TestResolveGalaxyRequirements_NothingExists(t *testing.T) {
+	dir := t.TempDir()
+	logger := &collectingLogger{}
+	app := newTestApp(dir, logger)
+
+	rolePaths, collectionPaths := app.resolveGalaxyRequirements()
+
+	assert.Empty(t, rolePaths)
+	assert.Empty(t, collectionPaths)
+
+	// No roles/ or collections/ dirs => no warnings about them.
+	assert.Equal(t, 0, logger.countContaining("contains no requirements.yml"))
+
+	// No shared requirements.yml anywhere => exactly one "No requirements.yml found" message.
+	assert.Equal(t, 1, logger.countContaining("No requirements.yml found"))
+	// It should list the searched path.
+	assert.Contains(t, logger.joined(), path.Join(dir, "requirements.yml"))
+}
+
+func TestResolveGalaxyRequirements_SubdirExistsButNoFile(t *testing.T) {
+	dir := t.TempDir()
+	mustMkdir(t, path.Join(dir, "roles"))
+	mustMkdir(t, path.Join(dir, "collections"))
+
+	logger := &collectingLogger{}
+	app := newTestApp(dir, logger)
+
+	rolePaths, collectionPaths := app.resolveGalaxyRequirements()
+
+	assert.Empty(t, rolePaths)
+	assert.Empty(t, collectionPaths)
+
+	// Each existing-but-empty subdir produces exactly one warning.
+	assert.Equal(t, 1, logger.countContaining(path.Join(dir, "roles")+" exists but contains no requirements.yml"))
+	assert.Equal(t, 1, logger.countContaining(path.Join(dir, "collections")+" exists but contains no requirements.yml"))
+
+	// Still no shared root requirements.yml => one "not found" message.
+	assert.Equal(t, 1, logger.countContaining("No requirements.yml found"))
+}
+
+func TestResolveGalaxyRequirements_SubdirFilesExist(t *testing.T) {
+	dir := t.TempDir()
+	rolesReq := path.Join(dir, "roles", "requirements.yml")
+	collectionsReq := path.Join(dir, "collections", "requirements.yml")
+	mustWrite(t, rolesReq)
+	mustWrite(t, collectionsReq)
+
+	logger := &collectingLogger{}
+	app := newTestApp(dir, logger)
+
+	rolePaths, collectionPaths := app.resolveGalaxyRequirements()
+
+	assert.Equal(t, []string{rolesReq}, rolePaths)
+	assert.Equal(t, []string{collectionsReq}, collectionPaths)
+
+	// Subdirs have their files => no warnings.
+	assert.Equal(t, 0, logger.countContaining("contains no requirements.yml"))
+
+	// No shared root requirements.yml => one "not found" message (subdir files are separate).
+	assert.Equal(t, 1, logger.countContaining("No requirements.yml found"))
+}
+
+func TestResolveGalaxyRequirements_SharedRootFile(t *testing.T) {
+	dir := t.TempDir()
+	sharedReq := path.Join(dir, "requirements.yml")
+	mustWrite(t, sharedReq)
+
+	logger := &collectingLogger{}
+	app := newTestApp(dir, logger)
+
+	rolePaths, collectionPaths := app.resolveGalaxyRequirements()
+
+	// Shared file is offered to BOTH types.
+	assert.Equal(t, []string{sharedReq}, rolePaths)
+	assert.Equal(t, []string{sharedReq}, collectionPaths)
+
+	// Shared file exists => NO "not found" message at all.
+	assert.Equal(t, 0, logger.countContaining("No requirements.yml found"))
+	// And no subdir warnings (no roles/ or collections/ dirs).
+	assert.Equal(t, 0, logger.countContaining("contains no requirements.yml"))
+}
+
+func TestResolveGalaxyRequirements_AllPresent(t *testing.T) {
+	dir := t.TempDir()
+	rolesReq := path.Join(dir, "roles", "requirements.yml")
+	collectionsReq := path.Join(dir, "collections", "requirements.yml")
+	sharedReq := path.Join(dir, "requirements.yml")
+	mustWrite(t, rolesReq)
+	mustWrite(t, collectionsReq)
+	mustWrite(t, sharedReq)
+
+	logger := &collectingLogger{}
+	app := newTestApp(dir, logger)
+
+	rolePaths, collectionPaths := app.resolveGalaxyRequirements()
+
+	// roles: subdir file + shared file.
+	assert.Contains(t, rolePaths, rolesReq)
+	assert.Contains(t, rolePaths, sharedReq)
+	assert.Len(t, rolePaths, 2)
+
+	// collections: subdir file + shared file.
+	assert.Contains(t, collectionPaths, collectionsReq)
+	assert.Contains(t, collectionPaths, sharedReq)
+	assert.Len(t, collectionPaths, 2)
+
+	// Nothing missing => no warnings, no "not found".
+	assert.Equal(t, 0, logger.countContaining("contains no requirements.yml"))
+	assert.Equal(t, 0, logger.countContaining("No requirements.yml found"))
+}