govulncheck: run from a Container

[xcoulon]

• drop the composite action (hence the checkout needs to be added in the
  workflow)
• specify the image to run in action.yaml
• build and publish a new image when PRs for this are merged

See updated usage in codeready-toolchain/host-operator#1219

Signed-off-by: Xavier Coulon xcoulon@redhat.com

Summary by CodeRabbit

• New Features

  • Added debug mode and structured logging with more detailed runtime output; vulnerability results now print to standard output.
  • CLI: config flag marked required and repository-root path handling clarified; new debug flag available.

• Chores

  • Action now runs via a Docker image; build and publish workflow added to push container images.
  • Container build definition added and Go toolchain updated to 1.23.
  • CI: lint and test split into separate jobs and triggers simplified.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Refactors the action from a composite to a Docker-based action, adds Containerfile and publish workflow to build/push a quay.io image, upgrades Go to 1.23, and migrates internal logging/output and Scan API to structured slog/io.Writer with corresponding test updates.

Changes

Cohort / File(s)	Summary
GitHub Actions — publish \.github/workflows/govulncheck-action-publish.yml	New workflow that builds and pushes quay.io/codeready-toolchain/govulncheck-action on pushes to master using Buildah; computes short SHA, tags (latest + SHA), logs into Quay via secret, and prints registry path.
GitHub Actions — test & lint \.github/workflows/govulncheck-action-test-lint.yml	CI triggers simplified to pull_request on master; lint extracted to a separate job using golangci/golangci-lint-action@v9 with a checkout step; test job runs make test on ubuntu-latest.
Container build govulncheck-action/Containerfile	Added multi-stage Containerfile: builder (golang:1.23) compiles govulncheck with GOOS/GOARCH args; final stage copies binary to /usr/local/bin and sets entrypoint.
Action manifest govulncheck-action/action.yaml	Converted from composite to Docker-based action (docker://quay.io/codeready-toolchain/govulncheck-action:latest); removed go-version-file and cache inputs, added debug input, changed path default to /github/workspace, and pass --path, --config, --debug as container args.
Go toolchain govulncheck-action/go.mod	Bumped Go version to 1.23.0 and toolchain to go1.23.12.
CLI & logging govulncheck-action/cmd/root.go	Replaced stdlib logging with slog, added --debug flag, log working directory and go env GOMOD, and call govulncheck.Scan with a slog logger and updated parameters.
Core scan API govulncheck-action/internal/govulncheck/scan.go	Public API switched to *slog.Logger, reordered Scan params (path before config), changed ScanFunc signature, converted DefaultScan from a var to func DefaultScan(stderr io.Writer) ScanFunc, added debug directory listing and explicit stderr capture/printing.
Scan tests govulncheck-action/internal/govulncheck/scan_test.go	Tests updated to use *slog.Logger and the new Scan signature (path then config); adjusted logger initialization and call sites.
Vulnerability output govulncheck-action/internal/govulncheck/vulnerability.go	Print functions now accept io.Writer (stdout) instead of *log.Logger; pruneIgnoredVulns accepts *slog.Logger and emits structured warnings; outputs use fmt to the provided writer.
Vulnerability tests govulncheck-action/internal/govulncheck/vulnerability_test.go	Tests updated to pass an io.Writer to PrintVulnerabilities and align with slog-based logging changes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Pay attention to API signature changes for Scan/ScanFunc and all callers (scan.go, cmd/root.go, tests).
• Verify slog usage and structured fields in cmd/root.go and pruneIgnoredVulns.
• Validate DefaultScan stderr capture/printing and debug directory listing behavior.
• Confirm Containerfile build args and that action.yaml correctly maps inputs to container args.
• Review publish workflow secret handling, image tagging, and push steps.

Possibly related PRs

• govulncheck: also report outdated vulns #158 — Directly related changes to govulncheck Scan and vulnerability reporting (outdated vulnerabilities handling).
• govulncheck: path to go.mod #156 — Related edits to govulncheck-action/action.yaml and handling of Go version/setup-go behavior.

Suggested reviewers

• MatousJobanek
• mfrancisc
• alexeykazakov

Poem

🐰 I built a binary, crisp and bright,
Packed it in Docker through the night,
Logs now whisper tidy and clear,
Quay keeps the image snug and near,
Hop — the scanner’s ready, let’s cheer! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main change: converting the govulncheck GitHub Action from a composite action to run from a container image.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[rsoaresd]

Amazing work! 🚀

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (4)

.github/workflows/govulncheck-action-publish.yml (1)

20-26: Specify build context for the Containerfile.

The buildah-build action uses containerfiles: govulncheck-action/Containerfile, but the COPY . . instruction in the Containerfile expects the build context to be the govulncheck-action directory. Without an explicit context parameter, the build context defaults to the repository root, which may copy unintended files or cause the build to fail if relative paths don't resolve correctly.

       - name: Build image
         id: build-image
         uses: redhat-actions/buildah-build@v2
         with:
           image: codeready-toolchain/govulncheck-action
           tags: latest ${{ env.COMMIT_SHORT_SHA }}
+          context: govulncheck-action
           containerfiles: govulncheck-action/Containerfile

govulncheck-action/Containerfile (1)

3-4: ARG declarations lack default values.

GOOS and GOARCH are declared but not provided with defaults and aren't being passed in the GitHub Actions workflow. This means Go will use the host's OS/architecture, which should work for the current use case. Consider either:

1. Removing these ARGs if cross-compilation isn't needed, or
2. Setting sensible defaults like ARG GOOS=linux and ARG GOARCH=amd64.

-ARG GOOS
-ARG GOARCH
+ARG GOOS=linux
+ARG GOARCH=amd64

govulncheck-action/action.yaml (1)

17-18: Consider pinning the Docker image to a specific version or digest.

Using :latest tag for the container image can lead to non-reproducible builds. When the image is updated, workflows may behave differently without any code change. Consider using a semantic version tag or a SHA digest for better reproducibility.

   using: 'docker'
-  image: 'docker://quay.io/codeready-toolchain/govulncheck-action:latest'
+  image: 'docker://quay.io/codeready-toolchain/govulncheck-action:v1.0.0'  # or use SHA digest

govulncheck-action/internal/govulncheck/scan.go (1)

32-44: Minor optimization: ReadDir is called even when debug logging is disabled.

The directory listing is only used when debug level is enabled, but os.ReadDir is always called. This adds unnecessary I/O overhead in non-debug mode.

 func DefaultScan(stderr io.Writer) ScanFunc {
 	return func(ctx context.Context, logger *slog.Logger, path string) ([]byte, error) {
 		// check that the path exists
 		logger.Info("scanning for vulnerabilities", "path", path)
-		files, err := os.ReadDir(path)
-		if err != nil {
-			return nil, err
-		}
 		if logger.Enabled(ctx, slog.LevelDebug) {
+			files, err := os.ReadDir(path)
+			if err != nil {
+				return nil, err
+			}
 			for _, file := range files {
 				logger.Debug(file.Name())
 			}
 		}

Note: If path validation is important regardless of debug mode, consider using os.Stat(path) instead, which is lighter than ReadDir.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2ade4a5 and ea5660d.

📒 Files selected for processing (10)

• .github/workflows/govulncheck-action-publish.yml (1 hunks)
• .github/workflows/govulncheck-action-test-lint.yml (3 hunks)
• govulncheck-action/Containerfile (1 hunks)
• govulncheck-action/action.yaml (1 hunks)
• govulncheck-action/cmd/root.go (3 hunks)
• govulncheck-action/go.mod (1 hunks)
• govulncheck-action/internal/govulncheck/scan.go (2 hunks)
• govulncheck-action/internal/govulncheck/scan_test.go (11 hunks)
• govulncheck-action/internal/govulncheck/vulnerability.go (3 hunks)
• govulncheck-action/internal/govulncheck/vulnerability_test.go (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (5)

govulncheck-action/internal/govulncheck/vulnerability_test.go (2)

govulncheck-action/internal/configuration/configuration.go (1)

• New (20-31)

govulncheck-action/internal/govulncheck/vulnerability.go (1)

• PrintVulnerabilities (111-133)

govulncheck-action/internal/govulncheck/scan_test.go (2)

govulncheck-action/internal/configuration/configuration.go (2)

• New (20-31)
• Configuration (10-12)

govulncheck-action/internal/govulncheck/scan.go (1)

• Scan (15-28)

govulncheck-action/internal/govulncheck/vulnerability.go (2)

govulncheck-action/internal/configuration/configuration.go (1)

• Vulnerability (14-18)

govulncheck-action/internal/govulncheck/types.go (1)

• Vulnerability (39-46)

govulncheck-action/internal/govulncheck/scan.go (2)

govulncheck-action/internal/configuration/configuration.go (2)

• Configuration (10-12)
• Vulnerability (14-18)

govulncheck-action/internal/govulncheck/types.go (1)

• Vulnerability (39-46)

govulncheck-action/cmd/root.go (3)

govulncheck-action/internal/configuration/configuration.go (1)

• New (20-31)

govulncheck-action/internal/govulncheck/scan.go (2)

• Scan (15-28)
• DefaultScan (32-59)

govulncheck-action/internal/govulncheck/vulnerability.go (2)

• PrintVulnerabilities (111-133)
• PrintOutdatedVulnerabilities (135-139)

🔇 Additional comments (18)

govulncheck-action/go.mod (1)

3-5: LGTM!

The Go version and toolchain updates to 1.23 are consistent with the Containerfile base image (golang:1.23) and align with the broader containerization changes in this PR.

.github/workflows/govulncheck-action-publish.yml (1)

1-44: LGTM on overall workflow structure.

The workflow correctly triggers on master pushes to the govulncheck-action directory, builds and tags the image appropriately, and pushes to quay.io with proper secret handling.

govulncheck-action/Containerfile (1)

13-20: LGTM on using the full Go image.

Using golang:1.23 for the final stage is appropriate since the underlying govulncheck command requires the Go toolchain to analyze Go modules and binaries.

govulncheck-action/internal/govulncheck/vulnerability_test.go (3)

5-5: LGTM on slog migration.

The import change from log to log/slog aligns with the broader logging refactor in this PR.

---

60-60: LGTM on logger initialization.

The slog logger setup for test cases is correct and consistent with the new API.

---

192-242: LGTM on PrintVulnerabilities test update.

The test correctly uses bytes.Buffer as an io.Writer to capture and verify output, aligning with the updated PrintVulnerabilities(stdout io.Writer, vulns []*Vulnerability) signature.

.github/workflows/govulncheck-action-test-lint.yml (1)

36-48: Good separation of lint into its own job.

Running lint as a separate parallel job improves CI feedback time. The job structure with its own checkout step is correct.

govulncheck-action/cmd/root.go (2)

37-44: LGTM on slog configuration.

The structured logging setup with a debug flag toggle is well-implemented. Using slog.LevelInfo as default with optional slog.LevelDebug is a sensible approach.

---

73-81: LGTM on CLI flag definitions.

Marking --config and --path as required with explicit error handling for the unlikely case of MarkFlagRequired failure is appropriate.

govulncheck-action/internal/govulncheck/scan_test.go (2)

5-5: LGTM on slog import.

The import change to log/slog is consistent with the broader logging refactor.

---

17-170: LGTM on test updates.

All test cases have been correctly updated to use *slog.Logger in the scan function signatures and govulncheck.Scan calls. The test coverage for various vulnerability scenarios (no vulns, vulns found, ignored, expired, outdated) is comprehensive.

govulncheck-action/internal/govulncheck/vulnerability.go (3)

5-6: LGTM on import changes.

The migration from standard log to log/slog and the addition of io for writer-based output aligns well with the containerization approach and structured logging practices.

---

77-95: LGTM on slog migration for pruneIgnoredVulns.

The structured logging with slog.Warn and key-value pairs ("vuln-id", "silence-until") improves log parseability and is consistent with the broader logging refactor.

---

135-139: LGTM on PrintOutdatedVulnerabilities signature change.

The migration to io.Writer is consistent with PrintVulnerabilities and allows for flexible output redirection.

govulncheck-action/action.yaml (2)

11-14: LGTM on the debug input addition.

The debug flag is properly defined with sensible defaults and integrates well with the slog-based logging in the Go code.

---

19-22: LGTM on argument passing.

The arguments are correctly mapped from inputs to the container command-line interface.

govulncheck-action/internal/govulncheck/scan.go (2)

15-28: LGTM on Scan function signature update.

The migration to *slog.Logger is consistent with the broader refactor. The function correctly delegates to the scan function and applies pruning/listing of vulnerabilities.

---

46-57: LGTM on govulncheck command execution.

The command setup with buffer capture and proper error handling is well-implemented. Printing stdout to stderr on failure aids debugging failed scans.

[MatousJobanek]

Thanks

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

govulncheck-action/Containerfile (2)

1-12: Builder stage is correct; consider layering for better build caching

The builder stage is functionally fine as-is. If build time becomes an issue, you could improve cache reuse by copying go.mod/go.sum first and running go mod download in a separate layer before copying the rest of the sources, so dependency downloads are cached across source changes.

---

13-20: Runtime image works; optionally switch to a slimmer/non‑root runtime

Using golang:1.23 for the runtime is straightforward and safe for CGO, but it yields a relatively large image and runs as root by default. If you want to optimize later, you could:

• Use a smaller base (e.g., a minimal/runtime image) that still satisfies any CGO needs, and/or
• Add a non‑root USER for slightly better security posture.

These are optional and can be deferred.

govulncheck-action/internal/govulncheck/scan.go (1)

30-60: Consider tightening path validation and error context in DefaultScan

The ScanFunc type and DefaultScan implementation look functionally sound (govulncheck is called with -C path -format json ./..., stdout captured, stderr streamed, stdout dumped to stderr on failure). Two small robustness tweaks you might consider:

• After os.Stat(path), verify that path is actually a directory before calling os.ReadDir(path) in debug mode; today, a file path will pass Stat but fail ReadDir.
• Wrap os.Stat/os.ReadDir errors with path context so failures are easier to interpret in logs.

For example:

 func DefaultScan(stderr io.Writer) ScanFunc {
 	return func(ctx context.Context, logger *slog.Logger, path string) ([]byte, error) {
-		// check that the path exists
-		logger.Info("scanning for vulnerabilities", "path", path)
-		if _, err := os.Stat(path); err != nil {
-			return nil, err
-		}
+		// check that the path exists and is a directory
+		logger.Info("scanning for vulnerabilities", "path", path)
+		info, err := os.Stat(path)
+		if err != nil {
+			return nil, fmt.Errorf("invalid scan path %q: %w", path, err)
+		}
+		if !info.IsDir() {
+			return nil, fmt.Errorf("scan path %q is not a directory", path)
+		}
 		if logger.Enabled(ctx, slog.LevelDebug) {
-			files, err := os.ReadDir(path)
-			if err != nil {
-				return nil, err
-			}
+			files, err := os.ReadDir(path)
+			if err != nil {
+				return nil, fmt.Errorf("failed to read directory %q: %w", path, err)
+			}
 			for _, file := range files {
 				logger.Debug(file.Name())
 			}
 		}

Not required for correctness, but they make failures more self-explanatory and guard against non-directory paths.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between edb3e44 and 4ce609e.

📒 Files selected for processing (2)

• govulncheck-action/Containerfile (1 hunks)
• govulncheck-action/internal/govulncheck/scan.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

govulncheck-action/internal/govulncheck/scan.go (2)

govulncheck-action/internal/configuration/configuration.go (2)

• Configuration (10-12)
• Vulnerability (14-18)

govulncheck-action/internal/govulncheck/types.go (1)

• Vulnerability (39-46)

🔇 Additional comments (2)

govulncheck-action/internal/govulncheck/scan.go (2)

3-13: slog/io imports and wiring look consistent with the new logging model

Importing log/slog and io here is consistent with the new Scan/DefaultScan signatures and usage; nothing stands out as problematic.

---

15-28: Scan orchestration and return values look correct after signature change

The reordered Scan signature and use of ScanFunc are coherent: you invoke scan(ctx, logger, path), parse with getVulnerabilities, then return pruned and outdated slices in a clear way. As long as all call sites are updated to the new (ctx, logger, scan, path, config) order and *slog.Logger, this looks good.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

govulncheck-action/internal/govulncheck/scan.go (1)

43-51: Optional: Consider logging directory summary instead of all files.

The debug file listing logs every file name individually, which could be verbose for large directories. Consider logging a count summary or limiting the output.

Example alternative:

 if logger.Enabled(ctx, slog.LevelDebug) {
   files, err := os.ReadDir(path)
   if err != nil {
     return nil, fmt.Errorf("failed to read directory: %w", err)
   }
-  for _, file := range files {
-    logger.Debug(file.Name())
-  }
+  logger.Debug("directory contents", "file_count", len(files))
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4ce609e and a82ad56.

📒 Files selected for processing (1)

• govulncheck-action/internal/govulncheck/scan.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

govulncheck-action/internal/govulncheck/scan.go (2)

govulncheck-action/internal/configuration/configuration.go (2)

• Configuration (10-12)
• Vulnerability (14-18)

govulncheck-action/internal/govulncheck/types.go (1)

• Vulnerability (39-46)

🔇 Additional comments (3)

govulncheck-action/internal/govulncheck/scan.go (3)

3-13: LGTM! Imports align with slog migration.

The addition of io and log/slog supports the migration from the legacy log.Logger to structured logging and writer-based output.

---

15-30: LGTM! Logger migration is consistent.

The function signature and ScanFunc type correctly migrate to *slog.Logger, maintaining the same scanning logic.

---

52-64: LGTM! Command execution and error handling are well-structured.

The implementation properly:

• Captures stdout and stderr separately
• Prints stdout to stderr on command failure for debugging
• Returns descriptive errors with context

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Fix incorrect error wrapping.

The error wraps err which is nil at this point (since os.Stat succeeded on line 36). Wrapping a nil error is incorrect.

Apply this diff:

 if !info.IsDir() {
-  return nil, fmt.Errorf("path is not a directory: %w", err)
+  return nil, fmt.Errorf("path is not a directory: %s", path)
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if !info.IsDir() {
	return nil, fmt.Errorf("path is not a directory: %w", err)
	}
	if !info.IsDir() {
	return nil, fmt.Errorf("path is not a directory: %s", path)
	}

🤖 Prompt for AI Agents

In govulncheck-action/internal/govulncheck/scan.go around lines 40 to 42, the
error creation wraps a nil err (from a prior successful os.Stat) which is
incorrect; replace the fmt.Errorf("path is not a directory: %w", err) call with
a plain error that does not wrap nil (e.g., fmt.Errorf("path is not a
directory") or fmt.Errorf("path %s is not a directory", path)) so the error
message is accurate and does not attempt to wrap a nil error.