From 71514bc3a12378dca6df9ac37c0794cecc48dbbd Mon Sep 17 00:00:00 2001 From: ksp Date: Sat, 30 May 2026 17:58:00 +0300 Subject: [PATCH] fix: reduce galaxy requirements check noise Skip silently when roles/ or collections/ subdirectory doesn't exist. Warn when subdirectory exists but has no requirements.yml. Log a single message listing searched paths when no shared requirements.yml is found, instead of one line per checked path. --- db_lib/AnsibleApp.go | 147 +++++++++++++++++++++-------- db_lib/AnsibleApp_test.go | 188 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 296 insertions(+), 39 deletions(-) create mode 100644 db_lib/AnsibleApp_test.go diff --git a/db_lib/AnsibleApp.go b/db_lib/AnsibleApp.go index bd38c0776d..7025ee5e31 100644 --- a/db_lib/AnsibleApp.go +++ b/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: +// - /roles/requirements.yml and /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. +// - /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. +// +// 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: /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: /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 { diff --git a/db_lib/AnsibleApp_test.go b/db_lib/AnsibleApp_test.go new file mode 100644 index 0000000000..7f20f6138b --- /dev/null +++ b/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")) +}