Repository navigation
fix(list): gate list instances --upload on settings.pro.enabled - #2330
Merged
Merged
Conversation
…ection.enabled Changes the Pro-enabled filter used by `atmos list instances --upload` to check `settings.pro.enabled == true` (strict boolean) instead of the narrower `settings.pro.drift_detection.enabled == true`. Updates tests, benchmarks, and the command reference docs. Behavior change: components that previously qualified via only `settings.pro.drift_detection.enabled: true` will now be excluded from upload unless they also set `settings.pro.enabled: true`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
April 16, 2026 18:15
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
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:
📝 WalkthroughWalkthroughChanged Pro eligibility to require Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (user)
participant Processor as Instance Processor
participant Uploader as Pro Upload Client
participant TUI as TUI (feedback)
CLI->>Processor: collect instances
Processor->>Processor: isProEnabled(instance) / countEnabledDisabled(all)
Processor->>Uploader: send full instances slice (preserve settings.pro.enabled)
Uploader->>Processor: upload result (success/failure)
Processor->>TUI: render success with totals (total, enabled, disabled, drift)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/list/list_instances_integration_test.go (1)
162-266: Consider table-driving these new Pro edge cases.The added scenarios are solid, but converting this block to a table-driven test would reduce duplication and make future permutations easier to extend.
As per coding guidelines, "Use table-driven tests for testing multiple scenarios in Go".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/list/list_instances_integration_test.go` around lines 162 - 266, The test block adds several independent scenarios for filterProEnabledInstances and should be refactored into a single table-driven test: create a slice of test cases (with fields like name string, instances []schema.Instance, wantLen int, wantFirstComponent string, wantFirstStack string) containing each scenario (invalid pro structure, drift_detection only, pro.enabled false, pro.enabled true, etc.), then loop over them with t.Run(tc.name, func(t *testing.T) { filtered := filterProEnabledInstances(tc.instances); assert.Len/Empty or assert.Equal checks based on tc.wantLen and optional expected component/stack fields }); ensure you reuse existing assertions (assert.Empty, assert.Len, assert.Equal) and keep unique scenario inputs identical to the current individual test cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/list/list_instances_integration_test.go`:
- Around line 162-266: The test block adds several independent scenarios for
filterProEnabledInstances and should be refactored into a single table-driven
test: create a slice of test cases (with fields like name string, instances
[]schema.Instance, wantLen int, wantFirstComponent string, wantFirstStack
string) containing each scenario (invalid pro structure, drift_detection only,
pro.enabled false, pro.enabled true, etc.), then loop over them with
t.Run(tc.name, func(t *testing.T) { filtered :=
filterProEnabledInstances(tc.instances); assert.Len/Empty or assert.Equal checks
based on tc.wantLen and optional expected component/stack fields }); ensure you
reuse existing assertions (assert.Empty, assert.Len, assert.Equal) and keep
unique scenario inputs identical to the current individual test cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75ed37a4-f061-4a1b-9f66-b39c5d2218fd
📒 Files selected for processing (7)
pkg/list/list_instances.gopkg/list/list_instances_bench_test.gopkg/list/list_instances_cmd_test.gopkg/list/list_instances_comprehensive_test.gopkg/list/list_instances_integration_test.gopkg/list/list_instances_pro_test.gowebsite/docs/cli/commands/list/list-instances.mdx
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2330 +/- ##
=======================================
Coverage 77.29% 77.30%
=======================================
Files 1084 1084
Lines 102410 102418 +8
=======================================
+ Hits 79159 79175 +16
+ Misses 18889 18881 -8
Partials 4362 4362
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Atmos Pro authentication is OIDC-only — users cannot mint `ATMOS_PRO_TOKEN` manually. Replace the incorrect hint with guidance matching our docs and the `describe affected` error path: require `id-token: write` in a GitHub Actions workflow and set `ATMOS_PRO_WORKSPACE_ID`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ver-side reconciliation
`atmos list instances --upload` now sends every real (non-abstract) instance
to Atmos Pro, with `settings.pro.enabled` preserved verbatim in the payload.
Previously, instances with `pro.enabled: false` (or no `pro` config) were
filtered out client-side, making "disabled" indistinguishable from "deleted"
on the server. Atmos Pro can now reconcile enabled/disabled state itself —
e.g., grey out disabled instances in the UI instead of losing track of them.
The success toast reports a tally:
Successfully uploaded 3 instances to Atmos Pro API (1 enabled, 2 disabled).
"No instances found; nothing to upload." only prints for empty repositories.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/list/list_instances.go (1)
334-335: Considerui.Successand pluralization for the toast.Two small, optional tweaks on this changed line:
- Per the UI/IO separation guideline, human-facing messages should go through
ui.*(stderr) rather thanu.PrintfMessageToTUI.ui.Successwould signal outcome semantically and keep this consistent with the siblingui.Info("No instances found; …")a few lines down.- The message renders "1 instances" when
len(instances) == 1. A tiny pluralize helper (or conditional) avoids the grammar nit in user output.♻️ Suggested tweak
- enabled, disabled := countEnabledDisabled(instances) - u.PrintfMessageToTUI("Successfully uploaded %d instances to Atmos Pro API (%d enabled, %d disabled).", len(instances), enabled, disabled) + enabled, disabled := countEnabledDisabled(instances) + noun := "instances" + if len(instances) == 1 { + noun = "instance" + } + ui.Success(fmt.Sprintf("Successfully uploaded %d %s to Atmos Pro API (%d enabled, %d disabled).", len(instances), noun, enabled, disabled))As per coding guidelines: "Use I/O layer (
pkg/io/) for stream access and UI layer (pkg/ui/) for formatting … ui.Write*/Writeln/Success/Error/Warning/Info/Markdown for human messages (stderr)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/list/list_instances.go` around lines 334 - 335, Replace the direct TUI print with the UI layer and fix pluralization: call ui.Success(...) instead of u.PrintfMessageToTUI(...) and construct the message using len(instances) with a conditional/plural helper so it says "1 instance" vs "N instances"; keep enabled/disabled from countEnabledDisabled(instances) and mirror the style used by ui.Info(...) nearby so all human-facing text flows through pkg/ui.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/list/list_instances.go`:
- Around line 334-335: Replace the direct TUI print with the UI layer and fix
pluralization: call ui.Success(...) instead of u.PrintfMessageToTUI(...) and
construct the message using len(instances) with a conditional/plural helper so
it says "1 instance" vs "N instances"; keep enabled/disabled from
countEnabledDisabled(instances) and mirror the style used by ui.Info(...) nearby
so all human-facing text flows through pkg/ui.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 136e7290-dda0-4deb-9d80-2d61292ee6c5
📒 Files selected for processing (8)
pkg/list/list_instances.gopkg/list/list_instances_bench_test.gopkg/list/list_instances_cmd_test.gopkg/list/list_instances_comprehensive_test.gopkg/list/list_instances_integration_test.gopkg/list/list_instances_pro_test.gopkg/list/list_instances_upload_test.gowebsite/docs/cli/commands/list/list-instances.mdx
✅ Files skipped from review due to trivial changes (1)
- pkg/list/list_instances_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/list/list_instances_comprehensive_test.go
- website/docs/cli/commands/list/list-instances.mdx
- pkg/list/list_instances_bench_test.go
- pkg/list/list_instances_cmd_test.go
The success message now reports how many uploaded instances have
drift detection enabled, making it easy to see drift coverage at
a glance after an upload:
Successfully uploaded 3 instances to Atmos Pro API (1 enabled, 2 disabled, 1 drift enabled).
Drift is counted independently of `pro.enabled`, since
`settings.pro.drift_detection.enabled` can be true even when
`settings.pro.enabled` is false.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/list/list_instances_pro_test.go (1)
13-17: Add a schema field compile guard.These tests reference
schema.Instance.Component,Stack, andSettings; add a package-level sentinel so field renames fail at compile time.Suggested guard
import ( "testing" "github.com/stretchr/testify/assert" "github.com/cloudposse/atmos/pkg/schema" ) + +var _ = schema.Instance{ + Component: "", + Stack: "", + Settings: map[string]any{}, +}As per coding guidelines, “Add compile-time sentinels for schema field references in tests.”
Also applies to: 21-28, 123-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/list/list_instances_pro_test.go` around lines 13 - 17, Add a package-level compile-time sentinel in pkg/list/list_instances_pro_test.go that references the specific schema fields used in tests so renames produce compile errors: create an unused variable that constructs a schema.Instance and accesses Component and Stack and also references schema.Instance.Settings (or schema.Settings) to touch Settings; this sentinel should live at top-level of the test file (outside TestIsProEnabled) and reference the schema.Instance and schema.Settings symbols so any future field rename will break compilation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/list/list_instances.go`:
- Around line 519-525: Filter the instances slice to only include those with
settings.pro.enabled == true before calling uploadInstances: compute a new slice
(e.g., proInstances) by iterating over instances and selecting only items where
instance.Settings.Pro.Enabled (or isProEnabled) is true, replace the
uploadCandidates check and the uploadInstances call to use proInstances, and
keep the existing "No instances found; nothing to upload." message but trigger
it when len(proInstances) == 0 so disabled/no-pro instances are not uploaded.
In `@website/docs/cli/commands/list/list-instances.mdx`:
- Around line 59-60: Update the doc text at the two incorrect spots to state
that the CLI uploads every real instance regardless of settings.pro.enabled,
preserving that flag in the payload so Atmos Pro can manage enabled/disabled
server-side; replace the current claim that only instances with
settings.pro.enabled: true are uploaded (lines referencing `--upload`) with
wording that matches the examples and the Go implementation (see
uploadInstancesWithDeps which does not filter by pro.enabled). Ensure both
occurrences (around the `--upload` dd and the other paragraph at line ~328)
mirror the examples: “All real instances are uploaded; settings.pro.enabled is
included in the payload so the server decides enabled vs. disabled.”
---
Nitpick comments:
In `@pkg/list/list_instances_pro_test.go`:
- Around line 13-17: Add a package-level compile-time sentinel in
pkg/list/list_instances_pro_test.go that references the specific schema fields
used in tests so renames produce compile errors: create an unused variable that
constructs a schema.Instance and accesses Component and Stack and also
references schema.Instance.Settings (or schema.Settings) to touch Settings; this
sentinel should live at top-level of the test file (outside TestIsProEnabled)
and reference the schema.Instance and schema.Settings symbols so any future
field rename will break compilation.
🪄 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: 7dcbd540-aa4b-4b4d-a4d3-9d431fd36c2f
📒 Files selected for processing (4)
pkg/list/list_instances.gopkg/list/list_instances_bench_test.gopkg/list/list_instances_pro_test.gowebsite/docs/cli/commands/list/list-instances.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/list/list_instances_bench_test.go
The `--upload` flag summary and the tip at the bottom still said "only instances with settings.pro.enabled: true are uploaded". That is no longer true — the CLI uploads every real instance and Atmos Pro reconciles enabled/disabled state from the preserved `settings.pro.enabled` flag in the payload. Brought both spots in line with the detailed example section and the actual implementation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
Apr 20, 2026
Contributor
|
These changes were released in v1.216.0-rc.2. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
atmos list instances --uploadto filter instances bysettings.pro.enabled == true(strict boolean) instead ofsettings.pro.drift_detection.enabled == true.isProDriftDetectionEnabled→isProEnabledand simplify the check to a single lookup onsettings.pro.enabled;drift_detection.enabledis no longer consulted.pro.enabled: truewithdrift_detection.enabled: falseis now enabled.website/docs/cli/commands/list/list-instances.mdxto document the filter criterion under--upload, in the examples section, and in the:::tipblock (noting it must be a boolean, not the string"true").why
settings.pro.enabled: trueconfigured on their components were hittingNo Atmos Pro-enabled instances found; nothing to upload.even when Pro was clearly enabled, because the filter required the narrowerdrift_detection.enabledsub-key.settings.pro.enabledis the correct top-level enablement flag for Pro; drift detection is one feature among several and shouldn't gate the whole upload.--uploadwithout specifying what made an instance eligible, so the failure mode was invisible to users.Behavior change (callout)
Components that previously qualified via only
settings.pro.drift_detection.enabled: true(withoutpro.enabled: true) will now be excluded from--upload. Users in that shape must addsettings.pro.enabled: true.references
--uploadwas introduced in feat(list): add matrix output format to list instances command #2322Summary by CodeRabbit
Bug Fixes
Documentation
Tests