Repository navigation
docs: add PRD for pro summary upload - #2259
Igor Rodionov (goruha) wants to merge 15 commits into
Conversation
Add product requirements document for uploading structured CI summary data (resource counts, terraform outputs, warnings, errors, output log) to Atmos Pro via the existing instance status PATCH endpoint. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a PRD and implements CLI/server-side support for uploading component-specific CI status metadata: new plugin interface Changes
Sequence Diagram(s)sequenceDiagram
actor CLI
participant Plugin as Component CI Plugin
participant Registry as Plugin Registry
participant Pro as Atmos Pro Server
participant Git as GitRepo
CLI->>Git: collect git info
CLI->>Registry: GetPlugin(component_type)
alt plugin found & implements StatusDataProvider
CLI->>Plugin: BuildStatusData(output, command)
Plugin-->>CLI: StatusData (map[string]any)
CLI->>CLI: mask sensitive values, base64-encode output_log, truncate from start if needed
CLI->>Pro: PATCH /api/v1/instances/{id}/status with component_type + metadata + command + exit_code
else plugin missing or build fails
CLI-->>CLI: emit warn-only
CLI->>Pro: PATCH /api/v1/instances/{id}/status with command + exit_code (no metadata)
end
Pro->>Pro: validate/accept payload (metadata optional)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 59-63: Update the PRD to require explicit secret-redaction and
payload-size limits for the CI output fields: enforce that ci.outputs and the
OutputLog (json:"output_log", referenced as OutputLog) must have sensitive
values masked or dropped (e.g., remove tokens, credentials, secrets), include a
description of the redaction strategy (masking pattern or drop-on-detect) and
add a max payload size limit with behavior on overflow (truncate output, include
truncation metadata and byte counts). Also apply the same requirements to the
other CI output sections noted (the other ci.outputs/ci.output_log occurrences)
so all mentions include the redaction rule, truncation policy, and metadata
requirements.
- Around line 133-140: The PRD shows an unsafe direct type assertion on
result.Data (e.g., result.Data.(*TerraformOutputData).Outputs) which can panic;
update the steps that map ParseOutput()'s OutputResult into InstanceStatusCI to
use a safe assertion pattern: call terraform.ParseOutput(), then check the
assertion like data, ok := result.Data.(*TerraformOutputData) and only read
data.ResourceCounts, data.Outputs (converting TerraformOutput.Value to raw
values), and data.Warnings when ok is true, otherwise set Defaults/empty values
and propagate result.Errors into InstanceStatusCI.Errors; mention the use of the
ok boolean and fallback behavior when mapping HasChanges, HasErrors,
ResourceCounts, Outputs, Warnings, and Errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec11f5ac-c3ff-484f-a00c-90d9c759185f
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2259 +/- ##
==========================================
+ Coverage 78.90% 78.91% +0.01%
==========================================
Files 1202 1207 +5
Lines 116155 116505 +350
==========================================
+ Hits 91648 91943 +295
- Misses 19441 19480 +39
- Partials 5066 5082 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…abstraction - Add business problem section explaining the user pain and value proposition - Output log must pass through IO masking layer (Gitleaks patterns) before base64 encoding - Introduce StatusDataProvider interface for component-type abstraction - Terraform is first implementation; helmfile/packer plug into same contract - Polymorphic ci.data field discriminated by component_type Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/prd/pro-summary-upload.md (2)
206-207:⚠️ Potential issue | 🟠 MajorDefine redaction rules for
ci.data.outputs(not justoutput_log).The PRD secures
output_log, butci.data.outputsis still documented as raw values. Terraform outputs can carry secrets, so this needs an explicit mask/omit rule tied to Terraform sensitive metadata before upload.Suggested PRD patch
- | `ci.data.outputs` | `TerraformOutputData.Outputs` | Terraform output values (raw) | + | `ci.data.outputs` | `TerraformOutputData.Outputs` | Terraform output values with sensitive values masked or omitted |- **Secret masking before upload**: The output log passes through `io.Mask()` (Gitleaks-based, 120+ patterns) before base64 encoding. This ensures secrets never leave the CLI unredacted, matching the security guarantees of all other Atmos output streams. + - **Sensitive Terraform outputs**: Values marked sensitive in Terraform output metadata MUST NOT be uploaded in cleartext. Replace with `"[REDACTED]"` or omit the key.Also applies to: 301-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/pro-summary-upload.md` around lines 206 - 207, Add explicit redaction rules for ci.data.outputs similar to output_log: detect TerraformOutputData.Outputs entries flagged as sensitive (Terraform "sensitive" metadata) and either mask or omit those values before any upload; update the PRD text to state that ci.data.outputs will be filtered according to Terraform sensitive metadata and show the same masking/omission policy as output_log (also apply the same change to the duplicate spots referenced around lines 301-302).
71-73:⚠️ Potential issue | 🟡 MinorAlign “apply-only outputs” requirement with the implementation snippet.
The prose says outputs are apply-only, but the snippet sets
Outputsunconditionally. Add an explicit command/result guard in the PRD snippet to avoid divergent implementations.Suggested PRD patch
- data.Data = &TerraformStatusData{ - ResourceCounts: &tfData.ResourceCounts, - Outputs: extractOutputValues(tfData.Outputs), - } + tfStatus := &TerraformStatusData{ + ResourceCounts: &tfData.ResourceCounts, + } + if isApplyCommand(result) { + tfStatus.Outputs = extractOutputValues(tfData.Outputs) + } + data.Data = tfStatusAlso applies to: 86-89, 202-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/pro-summary-upload.md` around lines 71 - 73, The PRD snippet unconditionally includes the Outputs field (Outputs map[string]any) even though the prose requires “apply-only outputs”; update the example so Outputs is only populated when the operation/result indicates an apply (e.g., add an explicit guard checking command == "apply" or result.IsApplySuccess before setting Outputs). Locate the struct/variable with the Outputs symbol and change the example flow to conditionally assign or include Outputs only in the apply-success branch (and show an empty/omitted Outputs for non-apply commands).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 206-207: Add explicit redaction rules for ci.data.outputs similar
to output_log: detect TerraformOutputData.Outputs entries flagged as sensitive
(Terraform "sensitive" metadata) and either mask or omit those values before any
upload; update the PRD text to state that ci.data.outputs will be filtered
according to Terraform sensitive metadata and show the same masking/omission
policy as output_log (also apply the same change to the duplicate spots
referenced around lines 301-302).
- Around line 71-73: The PRD snippet unconditionally includes the Outputs field
(Outputs map[string]any) even though the prose requires “apply-only outputs”;
update the example so Outputs is only populated when the operation/result
indicates an apply (e.g., add an explicit guard checking command == "apply" or
result.IsApplySuccess before setting Outputs). Locate the struct/variable with
the Outputs symbol and change the example flow to conditionally assign or
include Outputs only in the apply-success branch (and show an empty/omitted
Outputs for non-apply commands).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67057692-024d-48dd-b052-eb18788bf3f3
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
- Fix io.Mask() to use WithStdoutCapture (already-masked capture) - Clarify two capture paths: raw for parsing, masked for output_log - Replace sensitive terraform outputs with <MASKED> - Add server-defined output log size limits with truncation - Note deploy→apply conversion for Atmos Pro - Change CI field from typed struct to map[string]any for flexibility - Add cross-reference to instance-status-raw-upload.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/prd/pro-summary-upload.md (1)
177-203: Fix duplicate section numbering for readability.There are two
### 7.sections (“Integration Point” and “Data to Upload”). Renumber the second one to keep the doc easy to navigate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/pro-summary-upload.md` around lines 177 - 203, The document has duplicate heading numbers: both "Integration Point" and "Data to Upload" use "### 7."; rename the second heading (the one titled "Data to Upload") to "### 8." (or the next sequential number) so headings read "### 7. Integration Point" and "### 8. Data to Upload" to restore correct numbering and improve navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 216-217: Update the docs to remove the phrase "Full command
stdout" for `ci.output_log` and clarify that `ci.output_log` contains the
captured stdout which may be truncated (see `ci.truncated` and Section 6), e.g.,
describe it as "Captured command stdout (base64-encoded); may be partial if
truncated" and mention that truncation is controlled/masked by
`WithStdoutCapture`; ensure both `ci.output_log` and `ci.truncated` entries
consistently state the partial/truncation behavior.
- Around line 131-132: The doc incorrectly shows resolving the CI plugin with
ci.GetPlugin(info.Command); change this to use the component type string (e.g.,
info.ComponentType) when calling ci.GetPlugin so the plugin lookup succeeds for
components like "terraform" or "ansible"; then, if the resolved plugin
implements StatusDataProvider, call its BuildStatusData(result, output) to
obtain the map[string]any as described (reference symbols: ci.GetPlugin,
info.ComponentType, StatusDataProvider, BuildStatusData, result, output).
---
Nitpick comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 177-203: The document has duplicate heading numbers: both
"Integration Point" and "Data to Upload" use "### 7."; rename the second heading
(the one titled "Data to Upload") to "### 8." (or the next sequential number) so
headings read "### 7. Integration Point" and "### 8. Data to Upload" to restore
correct numbering and improve navigation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bb9c395-580f-4a14-a711-9e01d69a0e87
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 153-203: The document has duplicate section numbers for the
headings "Integration Point" and "Data to Upload"; update the heading numbers so
they are sequential by renaming "7. Integration Point" to "8. Integration Point"
and renaming the subsequent "7. Data to Upload" to "9. Data to Upload" (ensure
any in-text references or the table of contents that mention these section
numbers are updated accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c13d652-ef19-4ce7-a8e7-5474a09835a0
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
|
These changes were released in v1.214.0-test.0. |
* feat: upload structured CI data to Atmos Pro with instance status Add StatusDataProvider interface to CI plugin system, allowing each component type to contribute structured data to the Pro status upload. Terraform plugin implements it with resource counts, outputs (sensitive values masked), warnings, and errors. Output log is captured via WithStdoutCapture (already masked), base64-encoded, and truncated from the beginning if exceeding 3MB. The CI data is included as a flexible map[string]any on the existing InstanceStatusUploadRequest DTO, gated on ci.enabled. When disabled, payloads are identical to before (backward compatible). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: improve coverage for CI data payload and output log truncation - API client: verify CI block present/absent in serialized PATCH payload - Exec: verify output_log contains masked content (base64 decoded) - Exec: verify truncation at defaultMaxOutputLogBytes keeps tail, drops head - Exec: verify truncation flag set, boundary and under-limit cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: rename ci to metadata, promote component_type to first-class field - Rename DTO field from CI to Metadata (json:"metadata,omitempty") - Add ComponentType as top-level DTO field (json:"component_type,omitempty") - Remove component_type from plugin BuildStatusData return map - Update API client to include component_type and metadata in payload - Update uploadStatus signature to accept componentType and metadata - Add tests for captureOutput path with realistic terraform output: plan with resource counts, apply output, error output, masked tokens, empty output, and large output truncation - Update PRD to reflect all renames and new structure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: cover buildMetadataForUpload and captureOutput gate Extract buildMetadataForUpload() from executeMainTerraformCommand to make the captureOutput→buildCIStatusData path unit-testable. Add 4 test cases: captureOutput=false returns nil, captureOutput=true with terraform produces metadata+output_log, unknown plugin returns nil, empty output skips output_log key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it Igor Rodionov (@goruha)? 🙏 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/exec/pro.go (1)
217-218: Consider updating doc comment.The comment says "terraform results" but the function now handles multiple component types. A small wording tweak would improve clarity.
📝 Suggested doc update
-// uploadStatus uploads the terraform results to the pro API. +// uploadStatus uploads the component execution results to the pro API. func uploadStatus(info *schema.ConfigAndStacksInfo, exitCode int, componentType string, metadata map[string]any, client pro.AtmosProAPIClientInterface, gitRepo git.GitRepoInterface) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/pro.go` around lines 217 - 218, The doc comment for uploadStatus is outdated (mentions "terraform results" while the function handles multiple component types); update the comment above the uploadStatus(func uploadStatus(...)) to describe that it uploads operation results for various component types (use parameter names like componentType, info, exitCode, metadata) to the pro API so it's clear the function is not Terraform-specific. Keep the comment short and neutral (e.g., "uploadStatus uploads operation results for the given component type to the pro API").internal/exec/pro_test.go (1)
467-482: Conditional assertions may mask test failures.The
if result != nilpattern means assertions are skipped if the terraform plugin isn't registered. Tests pass vacuously in that case.Consider either:
- Adding a blank import
_ "github.com/cloudposse/atmos/pkg/ci/plugins/terraform"to ensure registration.- Using
require.NotNil(t, result)to fail fast if the precondition isn't met.Proposed fix to enforce plugin registration
import ( "encoding/base64" "testing" "time" gogit "github.com/go-git/go-git/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" atmosgit "github.com/cloudposse/atmos/pkg/git" "github.com/cloudposse/atmos/pkg/pro/dtos" "github.com/cloudposse/atmos/pkg/schema" + + // Import terraform plugin to trigger init() registration. + _ "github.com/cloudposse/atmos/pkg/ci/plugins/terraform" )Then update the test:
t.Run("returns CI data with output log for terraform", func(t *testing.T) { - // The terraform plugin is auto-registered via init(). - // We need to import the package to trigger registration. info := &schema.ConfigAndStacksInfo{ Command: "terraform", SubCommand: "plan", } output := []byte("Plan: 2 to add, 1 to change, 0 to destroy.") result := buildCIStatusData(info, output) - // If terraform plugin is registered, we should get data. - if result != nil { - assert.Contains(t, result, "output_log") - assert.Contains(t, result, "has_changes") - } + require.NotNil(t, result, "terraform plugin should be registered") + assert.Contains(t, result, "output_log") + assert.Contains(t, result, "has_changes") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/pro_test.go` around lines 467 - 482, The test uses a conditional "if result != nil" which masks failures when the terraform plugin isn't registered; update the test "returns CI data with output log for terraform" to enforce the precondition by replacing the conditional with require.NotNil(t, result) (from testify/require) immediately after calling buildCIStatusData(info, output) and then run the existing assert.Contains checks; alternatively ensure plugin registration by adding a blank import of the terraform plugin package, but the simpler change is to call require.NotNil(t, result) so the test fails fast if buildCIStatusData returned nil.docs/prd/pro-summary-upload.md (1)
165-165: Missing language specifier on code fence.Minor markdown issue—can be addressed in a follow-up cleanup per repository conventions.
Based on learnings: "For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/pro-summary-upload.md` at line 165, The code block in docs/prd/pro-summary-upload.md uses a triple-backtick fence (```) without a language specifier; update the opening fence for that code block to include the correct language (e.g., ```yaml, ```json, or ```bash) so markdownlint rules like MD040/MD010/MD034 are satisfied — locate the code fence with the ``` marker in pro-summary-upload.md and add the appropriate language token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/prd/pro-summary-upload.md`:
- Line 165: The code block in docs/prd/pro-summary-upload.md uses a
triple-backtick fence (```) without a language specifier; update the opening
fence for that code block to include the correct language (e.g., ```yaml,
```json, or ```bash) so markdownlint rules like MD040/MD010/MD034 are satisfied
— locate the code fence with the ``` marker in pro-summary-upload.md and add the
appropriate language token.
In `@internal/exec/pro_test.go`:
- Around line 467-482: The test uses a conditional "if result != nil" which
masks failures when the terraform plugin isn't registered; update the test
"returns CI data with output log for terraform" to enforce the precondition by
replacing the conditional with require.NotNil(t, result) (from testify/require)
immediately after calling buildCIStatusData(info, output) and then run the
existing assert.Contains checks; alternatively ensure plugin registration by
adding a blank import of the terraform plugin package, but the simpler change is
to call require.NotNil(t, result) so the test fails fast if buildCIStatusData
returned nil.
In `@internal/exec/pro.go`:
- Around line 217-218: The doc comment for uploadStatus is outdated (mentions
"terraform results" while the function handles multiple component types); update
the comment above the uploadStatus(func uploadStatus(...)) to describe that it
uploads operation results for various component types (use parameter names like
componentType, info, exitCode, metadata) to the pro API so it's clear the
function is not Terraform-specific. Keep the comment short and neutral (e.g.,
"uploadStatus uploads operation results for the given component type to the pro
API").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9045a5a1-23bd-464e-afc8-117bbc51525f
📒 Files selected for processing (12)
docs/prd/pro-summary-upload.mdinternal/exec/pro.gointernal/exec/pro_test.gointernal/exec/terraform_execute_helpers_exec.gopkg/ci/internal/plugin/types.gopkg/ci/plugin_registry.gopkg/ci/plugin_registry_test.gopkg/ci/plugins/terraform/plugin.gopkg/ci/plugins/terraform/plugin_test.gopkg/pro/api_client_instance_status.gopkg/pro/api_client_instance_status_test.gopkg/pro/dtos/instances.go
✅ Files skipped from review due to trivial changes (1)
- pkg/ci/internal/plugin/types.go
…into prd/pro-summary-upload * 'prd/pro-summary-upload' of github.com:cloudposse/atmos: [autofix.ci] apply automated fixes
|
These changes were released in v1.216.0-test.0. |
Resolves conflicts by combining CI output capture from pro-summary-upload with retry logic from main in executeMainCommand, and preserving both test suites in plugin_test.go. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
These changes were released in v1.221.0-test.0. |
|
These changes were released in v1.221.0-test.1. |
|
These changes were released in v1.221.0-test.2. |
|
💥 This pull request now has conflicts. Could you fix it Igor Rodionov (@goruha)? 🙏 |
|
💥 This pull request now has conflicts. Could you fix it Igor Rodionov (@goruha)? 🙏 |
Summary
InstanceStatusUploadRequestDTO with a nestedCIblock containing resource counts, terraform outputs, warnings, errors, and base64-encoded output log--upload-statusflag +ci.enabled— backward compatible viaomitemptypointerTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit