Skip to content

PF-173: Add logs as artifact for a job#229

Open
gruntwork-ci wants to merge 1 commit into
mainfrom
claude-pr/PF-173-28140438123
Open

PF-173: Add logs as artifact for a job#229
gruntwork-ci wants to merge 1 commit into
mainfrom
claude-pr/PF-173-28140438123

Conversation

@gruntwork-ci

@gruntwork-ci gruntwork-ci commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Implements plan for Linear issue PF-173.

Plan source: https://github.com/gruntwork-io-team/linear-claude-plans/blob/main/plans/PF-173.md


Claude's implementation summary
No fixture impact. Implementation complete. Let me provide the summary of changes per repo.

---

## Summary of changes

### `gruntwork-io/pipelines` (this repo)

Added `EXPERIMENT_LOGS_AS_ARTIFACT` opt-in experiment and the CLI plumbing to surface a per-unit Terragrunt log artifact path and "Download Logs" link.

- `internal/features/features.go` — added `LogsAsArtifact` constant + default-OFF entry.
- `internal/features/features_test.go` — added focused test asserting default is false (the existing AST-driven `TestAllFeaturesHaveCase` also picks it up).
- `internal/actions/terragrunt/terragruntexecutor.go` — added `LogsArtifactPath string \`output:"logs_artifact_path,omitempty"\`` to `TerragruntOutput`; populated only when the experiment is enabled and `--combined-output-path` was provided. Added a `combinedOutputAbsPath()` helper shared with `writeCombinedOutput`.
- `internal/step/step.go` — added `LogsArtifactURL string \`json:"logs_artifact_url,omitempty"\`` to `StepSummary`.
- `internal/comment/comment.go` — added `LogsArtifactURL` to `CommentInfraChange` and copied it from `step.Result` in `formatCommentData`.
- `internal/comment/templates/comment.gohtml` — render a `· <a href=...>Download Logs</a>` anchor next to "View Job Logs" when `LogsArtifactURL` is non-empty.
- `internal/comment/comment_test.go` — added two tests covering present-link and absent-link cases.
- `internal/actions/statusupdate/statusupdate.go` — added `logsArtifactURL` field to `Update`, new param on `NewUpdate`, populated `StepSummary.LogsArtifactURL`.
- `main.go` — added `--logs-artifact-url` CLI flag on `status-update update`, threaded into `NewUpdate`.
- `internal/actions/gitlabyml/templates/execute.goyml` — extended the generated child-pipeline execute job's `artifacts:` stanza to include `access: developer`, `expire_in: 30 days`, and the `pipelines-artifacts/` directory.
- `internal/actions/gitlabyml/gitlabdynamicchildren_test.go` — updated the `TestExecutePlan` snapshot to match the new artifacts block.
- `test/plumbing/statusupdate/update/update_test.go` and `test/porcelain/add-units-consolidated/add-unit-consolidated_test.go` — added the new `logsArtifactURL` constructor arg (empty string) to existing `NewUpdate(...)` call sites.

### `gruntwork-io/pipelines-workflows`

Added a conditional artifact upload step in the matrix execute jobs and passed the resulting URL through to the comment-job-update step.

- `.github/workflows/pipelines.yml` — added `Sanitize artifact name fragment` shell step (replaces `/` with `-` in the working directory) and `Upload unit logs as artifact` using `actions/upload-artifact@v6.0.0`, both gated on `steps.terragrunt.outputs.pipelines_logs_artifact_path != ''`. Added `logs_artifact_url: ${{ steps.upload_logs.outputs.artifact-url }}` to the `Update comment` step.
- `.github/workflows/pipelines-root.yml` — same change, with the AccountsRequested/AccountsAdded skip condition propagated to the new steps.

### `gruntwork-io/pipelines-actions`

Declared the new step output on the execute action and accepted/forwarded the new URL on the comment-job-update action.

- `.github/actions/pipelines-execute/action.yml` — declared `pipelines_logs_artifact_path` in the top-level `outputs:` block so the CLI's `OutputWriter`-emitted value bubbles to the calling workflow.
- `.github/actions/pipelines-comment-job-update/action.yml` — added `logs_artifact_url` input (default empty) and plumbed it into the composite's `env:` as `LOGS_ARTIFACT_URL`.
- `.github/actions/pipelines-comment-job-update/scripts/update.sh` — defaulted `LOGS_ARTIFACT_URL` to empty and appended `--logs-artifact-url "$LOGS_ARTIFACT_URL"` to the `pipelines status-update update` invocation.

### `gruntwork-io/docs`

- `docs/2.0/reference/pipelines/feature-flags.md` — documented `PIPELINES_FEATURE_EXPERIMENT_LOGS_AS_ARTIFACT` between `IGNORE_UNITS_WITHOUT_ENVIRONMENT` and `MINIMIZE_BLAST_RADIUS`, including a note about workflow/action version requirements and the dual env-var / HCL `env { }` enablement options.

### Missing / out-of-scope repos

The plan calls for changes in two GitLab-side repositories that are **not** present in this checked-out workspace:

- `gitlab.com/gruntwork-io/pipelines-workflows` (referred to as `gitlab-pipelines-workflows`) — the plan said no template changes were required here; only the `pipelines` CLI change (already applied to `internal/actions/gitlabyml/templates/execute.goyml`) was needed. **Effectively done from this repo.**
- `gitlab.com/gruntwork-io/pipelines-actions` (referred to as `pipelines-gitlab-actions`) — the plan required edits to `scripts/pipelines-execute.sh` to: extract `LogsArtifactPath` from the CLI JSON via `jq`, conditionally stage the log to `./pipelines-artifacts/`, derive the GitLab raw-artifact URL from `CI_SERVER_URL`/`CI_PROJECT_PATH`/`CI_JOB_ID`, and forward it as `--logs-artifact-url` to `status-update update`. **This repo is not checked out — these changes are skipped here and must be applied in a follow-up PR against `pipelines-gitlab-actions`.**

Tests run: `go build ./...`, `go vet ./...`, and `go test ./internal/features/ ./internal/comment/ ./internal/actions/gitlabyml/ ./internal/platform/outputs/ ./internal/step/` all pass. Two preexisting test failures (`TestTerragruntPlanOutDirNestsByUnitPath` and `TestExecuteTerragrunt`) are environmental — they require the `terragrunt` binary on `$PATH` and are unrelated to these changes.

Related PRs in this change set

Summary by CodeRabbit

  • New Features

    • Added downloadable logs to workflow runs, making Terragrunt unit logs easier to access after execution.
    • Updated run comments to include a direct link to the uploaded logs artifact.
  • Bug Fixes

    • Improved artifact naming to avoid issues with path characters and make uploads more reliable.
    • Added safer handling when logs are unavailable, preventing workflow interruptions.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Both workflow files now sanitize working-directory fragments, upload Terragrunt log files as GitHub Actions artifacts when logs exist, and pass the resulting artifact URL into the update-comment step.

Changes

Terragrunt log artifact wiring

Layer / File(s) Summary
Artifact upload and comment URL
.github/workflows/pipelines.yml, .github/workflows/pipelines-root.yml
A sanitized working-directory fragment is used for artifact naming, Terragrunt log files are uploaded with 30-day retention and overwrite enabled, and logs_artifact_url is forwarded to the comment step.

Sequence Diagram(s)

sequenceDiagram
  participant pipelines_execute_job
  participant terragrunt
  participant upload_logs
  participant update_comment
  pipelines_execute_job->>terragrunt: sets pipelines_logs_artifact_path
  pipelines_execute_job->>upload_logs: uploads logs when the path is non-empty
  upload_logs-->>pipelines_execute_job: artifact-url
  pipelines_execute_job->>update_comment: passes logs_artifact_url
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Logs took a stroll through tidy lanes,
With slashes trimmed and neatly strained,
An artifact bloomed in GitHub’s light,
And comments gained a link just right ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding job logs as an artifact.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude-pr/PF-173-28140438123

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/pipelines.yml (1)

283-292: 🩺 Stability & Availability | 🔵 Trivial

Download Logs link remains safe.

upload-artifact@v6.0.0 keeps the artifact-url output we need (plus artifact-id and artifact-digest), so the downstream step picking up the link is still on solid ground. The upgrade was mostly a runtime refresh, not an API break.

Just remember to keep that safety net for empty logs: if if-no-files-found: warn skips the upload, the URL will be empty, so the downstream step needs to handle a blank value gracefully.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/pipelines.yml around lines 283 - 292, The upload-artifact
step in pipelines.yml already exposes the artifact-url output, but the
downstream logic that consumes the logs link must explicitly handle the case
where no files were uploaded and the URL is empty. Update the step or follow-up
consumer that reads upload_logs/artifact-url to treat a blank value as valid and
skip or degrade gracefully instead of assuming a link always exists, using the
existing Upload unit logs as artifact step as the reference point.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In @.github/workflows/pipelines.yml:
- Around line 283-292: The upload-artifact step in pipelines.yml already exposes
the artifact-url output, but the downstream logic that consumes the logs link
must explicitly handle the case where no files were uploaded and the URL is
empty. Update the step or follow-up consumer that reads upload_logs/artifact-url
to treat a blank value as valid and skip or degrade gracefully instead of
assuming a link always exists, using the existing Upload unit logs as artifact
step as the reference point.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9980119c-4861-4e02-a48f-7eb95c23cefb

📥 Commits

Reviewing files that changed from the base of the PR and between eb28f2c and 9038346.

📒 Files selected for processing (2)
  • .github/workflows/pipelines-root.yml
  • .github/workflows/pipelines.yml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant