chore: Expand pure testing through venv#6090
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthrough
ChangesCatalog venv.Venv threading
Mem-exec test isolation and new hook/e2e tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
yhakbar
force-pushed
the
chore/thread-venv-through-cli
branch
from
d47a34d to
26a86cc
Compare
yhakbar
force-pushed
the
chore/expand-pure-testing-through-venv
branch
from
aa0c724 to
286d5b0
Compare
yhakbar
force-pushed
the
chore/thread-venv-through-cli
branch
3 times, most recently
from
73bcbb6 to
7b980aa
Compare
yhakbar
force-pushed
the
chore/thread-venv-through-cli
branch
from
7b980aa to
5d0e454
Compare
yhakbar
force-pushed
the
chore/expand-pure-testing-through-venv
branch
from
d6e09d2 to
9fed160
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/cli/commands/catalog/tui/redesign/update.go (3)
612-626:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse injected venv instead of global OSVenv() in module field discovery.
Line 615 calls
scaffold.Preparewithvenv.OSVenv(), which bypasses the model's injectedvenvand breaks pure testing. UpdatediscoverModuleFieldsto accept avenv.Venvparameter (as noted in the prior comment) and pass it toscaffold.Prepare.🔧 Suggested fix
-func discoverModuleFields(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, c *Component) tea.Msg { +func discoverModuleFields(ctx context.Context, l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) tea.Msg { quiet := l.WithOptions(log.WithOutput(io.Discard)) - plan, err := scaffold.Prepare(ctx, quiet, venv.OSVenv(), opts, c.TerraformSourcePath(), "") + plan, err := scaffold.Prepare(ctx, quiet, v, opts, c.TerraformSourcePath(), "") if err != nil { return formDiscoveryErrMsg{err: err} }🤖 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 `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 612 - 626, The discoverModuleFields function currently calls scaffold.Prepare with the global venv.OSVenv(), which ignores the injected venv and hampers testing; change discoverModuleFields signature to accept a venv.Venv parameter (e.g., add venv v venv.Venv), and pass that injected venv into scaffold.Prepare instead of venv.OSVenv(); update any callers to supply the injected venv and keep the rest of the logic (creating quiet logger, handling err -> formDiscoveryErrMsg, building fields with FieldsFromParsedVariables, and returning formReadyMsg with NewFormModel) unchanged.
631-648:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse venv-provided filesystem in values field discovery.
Line 637 constructs a new
vfs.NewOSFS()instead of using the venv-provided filesystem. This breaks pure testing by bypassing the injected environment. UpdatediscoverValuesFieldsto accept avenv.Venvparameter and usev.FS()forCollectValuesReferences.🔧 Suggested fix
-func discoverValuesFields(c *Component) tea.Msg { +func discoverValuesFields(v venv.Venv, c *Component) tea.Msg { configName := configFileForKind(c.Kind) if configName == "" { return formDiscoveryErrMsg{err: fmt.Errorf("component kind %q has no associated HCL file", c.Kind)} } - refs, err := CollectValuesReferences(vfs.NewOSFS(), filepath.Join(c.Repo.Path(), c.Dir, configName)) + refs, err := CollectValuesReferences(v.FS, filepath.Join(c.Repo.Path(), c.Dir, configName)) if err != nil { return formDiscoveryErrMsg{err: err} }🤖 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 `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 631 - 648, discoverValuesFields currently constructs vfs.NewOSFS() directly; change its signature to accept a venv.Venv (e.g., func discoverValuesFields(c *Component, v venv.Venv) tea.Msg) and replace vfs.NewOSFS() with v.FS() when calling CollectValuesReferences so the injected virtual filesystem is used; update any callers (e.g., where NewFormModel is invoked or discoverValuesFields is passed) to pass the venv instance.
575-599:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThread venv through the form discovery path.
The model has
m.venvbutenterFormStatecallsdiscoverFormCmdwithout passing it. This breaks the pure testing pattern established throughout this PR.discoverFormCmdshould accept avenv.Venvparameter and forward it to bothdiscoverModuleFieldsand the function signature at line 592.🔧 Suggested signature updates
-func enterFormState(m Model, c *Component, priorState sessionState) (tea.Model, tea.Cmd) { +func enterFormState(m Model, c *Component, priorState sessionState) (tea.Model, tea.Cmd) { m.priorState = priorState m.State = FormState m.form = nil m.scaffoldPlan = nil m.valuesRefs = nil m.selectedComponent = c - return m, discoverFormCmd(m.ctx, m.logger, m.terragruntOptions, c) + return m, discoverFormCmd(m.ctx, m.logger, m.venv, m.terragruntOptions, c) } -func discoverFormCmd(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, c *Component) tea.Cmd { +func discoverFormCmd(ctx context.Context, l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) tea.Cmd { return func() tea.Msg { if c.Kind.IsCopyable() { - return discoverValuesFields(c) + return discoverValuesFields(v, c) } - return discoverModuleFields(ctx, l, opts, c) + return discoverModuleFields(ctx, l, v, opts, c) } }🤖 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 `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 575 - 599, enterFormState currently calls discoverFormCmd without the model venv; update the discovery path to thread m.venv through: change discoverFormCmd signature to accept a venv.Venv (e.g., discoverFormCmd(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, c *Component, v venv.Venv) tea.Cmd), modify enterFormState to pass m.venv into that call, and inside discoverFormCmd forward the venv to the downstream calls (pass the venv into discoverModuleFields and, if applicable, discoverValuesFields) so the entire form discovery flow uses Model.m.venv. Ensure all call sites and tests are updated to the new signature.
🤖 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.
Outside diff comments:
In `@internal/cli/commands/catalog/tui/redesign/update.go`:
- Around line 612-626: The discoverModuleFields function currently calls
scaffold.Prepare with the global venv.OSVenv(), which ignores the injected venv
and hampers testing; change discoverModuleFields signature to accept a venv.Venv
parameter (e.g., add venv v venv.Venv), and pass that injected venv into
scaffold.Prepare instead of venv.OSVenv(); update any callers to supply the
injected venv and keep the rest of the logic (creating quiet logger, handling
err -> formDiscoveryErrMsg, building fields with FieldsFromParsedVariables, and
returning formReadyMsg with NewFormModel) unchanged.
- Around line 631-648: discoverValuesFields currently constructs vfs.NewOSFS()
directly; change its signature to accept a venv.Venv (e.g., func
discoverValuesFields(c *Component, v venv.Venv) tea.Msg) and replace
vfs.NewOSFS() with v.FS() when calling CollectValuesReferences so the injected
virtual filesystem is used; update any callers (e.g., where NewFormModel is
invoked or discoverValuesFields is passed) to pass the venv instance.
- Around line 575-599: enterFormState currently calls discoverFormCmd without
the model venv; update the discovery path to thread m.venv through: change
discoverFormCmd signature to accept a venv.Venv (e.g., discoverFormCmd(ctx
context.Context, l log.Logger, opts *options.TerragruntOptions, c *Component, v
venv.Venv) tea.Cmd), modify enterFormState to pass m.venv into that call, and
inside discoverFormCmd forward the venv to the downstream calls (pass the venv
into discoverModuleFields and, if applicable, discoverValuesFields) so the
entire form discovery flow uses Model.m.venv. Ensure all call sites and tests
are updated to the new signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13340220-b5f1-4f7b-b3c7-a72eb2026655
📒 Files selected for processing (30)
internal/cli/commands/catalog/catalog.gointernal/cli/commands/catalog/catalog_redesign.gointernal/cli/commands/catalog/catalog_test.gointernal/cli/commands/catalog/cli.gointernal/cli/commands/catalog/tui/model_test.gointernal/cli/commands/catalog/tui/redesign/load.gointernal/cli/commands/catalog/tui/redesign/model.gointernal/cli/commands/catalog/tui/redesign/model_test.gointernal/cli/commands/catalog/tui/redesign/scaffold.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/catalog/tui/redesign/view_test.gointernal/cli/commands/catalog/tui/redesign/welcome.gointernal/cli/commands/catalog/tui/redesign/welcome_test.gointernal/cli/commands/commands.gointernal/discovery/graph_target_test.gointernal/runner/run/action_with_hooks_test.gointernal/runner/run/creds/providers/externalcmd/provider_mem_test.gointernal/runner/run/download_source_test.gointernal/runner/run/hook_internal_test.gointernal/runner/run/hook_lifecycle_test.gointernal/runner/run/hook_test.gointernal/runner/run/run_e2e_test.gointernal/runner/run/version_check_mem_test.gointernal/services/catalog/catalog.gointernal/services/catalog/catalog_test.gointernal/services/catalog/module/repo.gointernal/services/catalog/module/repo_tag_test.gointernal/shell/git_mem_test.gopkg/config/config_helpers_hcl_mem_test.gotest/integration_catalog_test.go
yhakbar
force-pushed
the
chore/expand-pure-testing-through-venv
branch
from
82e2854 to
e9b5ad2
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/commands/catalog/catalog_redesign.go (1)
32-139: ⚖️ Poor tradeoffAcknowledge SonarCloud cognitive complexity flag.
SonarCloud reports cognitive complexity of 21 (vs. limit of 15). The complexity is inherent to the concurrent loading pattern: parallel discovery, bounded loader pool, mutex-protected error collection, and result aggregation. The function is well-commented and the control flow is clear. Extracting helpers would increase indirection without materially reducing complexity. Consider this acceptable for now, but monitor if the function grows further.
🤖 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 `@internal/cli/commands/catalog/catalog_redesign.go` around lines 32 - 139, SonarCloud flags runRedesign for high cognitive complexity due to the large anonymous function passed into redesign.RunRedesign; extract that inner block into a well-named helper (e.g., runRedesignDiscoveryAndLoad) to reduce nesting and make flow easier to read. Move the discovery and loading logic — the errgroup creation for discoverCatalogConfigURLs and discoverSourceFileURLs, the urlCh lifecycle, the loaders errgroup with SetLimit and its loaders.Go that calls redesign.LoadURL, and the failures collection (failuresMu, failures slice, and final aggregation/return of redesign.SourceLoadError) — into the new function, keeping the same signatures and behavior so runRedesign only calls redesign.RunRedesign with that helper as the callback.Source: Linters/SAST tools
🤖 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 `@internal/cli/commands/catalog/catalog_redesign.go`:
- Around line 32-139: SonarCloud flags runRedesign for high cognitive complexity
due to the large anonymous function passed into redesign.RunRedesign; extract
that inner block into a well-named helper (e.g., runRedesignDiscoveryAndLoad) to
reduce nesting and make flow easier to read. Move the discovery and loading
logic — the errgroup creation for discoverCatalogConfigURLs and
discoverSourceFileURLs, the urlCh lifecycle, the loaders errgroup with SetLimit
and its loaders.Go that calls redesign.LoadURL, and the failures collection
(failuresMu, failures slice, and final aggregation/return of
redesign.SourceLoadError) — into the new function, keeping the same signatures
and behavior so runRedesign only calls redesign.RunRedesign with that helper as
the callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24b19d5f-ca9e-428d-83d6-b69ce83403cb
📒 Files selected for processing (30)
internal/cli/commands/catalog/catalog.gointernal/cli/commands/catalog/catalog_redesign.gointernal/cli/commands/catalog/catalog_test.gointernal/cli/commands/catalog/cli.gointernal/cli/commands/catalog/tui/model_test.gointernal/cli/commands/catalog/tui/redesign/load.gointernal/cli/commands/catalog/tui/redesign/model.gointernal/cli/commands/catalog/tui/redesign/model_test.gointernal/cli/commands/catalog/tui/redesign/scaffold.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/catalog/tui/redesign/view_test.gointernal/cli/commands/catalog/tui/redesign/welcome.gointernal/cli/commands/catalog/tui/redesign/welcome_test.gointernal/cli/commands/commands.gointernal/discovery/graph_target_test.gointernal/runner/run/action_with_hooks_test.gointernal/runner/run/creds/providers/externalcmd/provider_mem_test.gointernal/runner/run/download_source_test.gointernal/runner/run/hook_internal_test.gointernal/runner/run/hook_lifecycle_test.gointernal/runner/run/hook_test.gointernal/runner/run/run_e2e_test.gointernal/runner/run/version_check_mem_test.gointernal/services/catalog/catalog.gointernal/services/catalog/catalog_test.gointernal/services/catalog/module/repo.gointernal/services/catalog/module/repo_tag_test.gointernal/shell/git_mem_test.gopkg/config/config_helpers_hcl_mem_test.gotest/integration_catalog_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/runner/run/creds/providers/externalcmd/provider_mem_test.go
- internal/runner/run/version_check_mem_test.go
- internal/shell/git_mem_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- internal/cli/commands/commands.go
- internal/cli/commands/catalog/tui/redesign/scaffold.go
- internal/cli/commands/catalog/tui/model_test.go
- internal/services/catalog/module/repo.go
- internal/cli/commands/catalog/tui/redesign/load.go
- internal/cli/commands/catalog/cli.go
- pkg/config/config_helpers_hcl_mem_test.go
- internal/runner/run/hook_internal_test.go
- test/integration_catalog_test.go
- internal/runner/run/run_e2e_test.go
- internal/cli/commands/catalog/catalog.go
- internal/cli/commands/catalog/tui/redesign/view_test.go
- internal/runner/run/download_source_test.go
- internal/services/catalog/catalog_test.go
- internal/services/catalog/catalog.go
- internal/runner/run/hook_lifecycle_test.go
- internal/cli/commands/catalog/tui/redesign/model_test.go
- internal/cli/commands/catalog/tui/redesign/welcome_test.go
- internal/cli/commands/catalog/tui/redesign/welcome.go
- internal/cli/commands/catalog/tui/redesign/model.go
- internal/discovery/graph_target_test.go
- internal/services/catalog/module/repo_tag_test.go
yhakbar
force-pushed
the
chore/expand-pure-testing-through-venv
branch
from
e9b5ad2 to
17510c1
Compare
yhakbar
force-pushed
the
chore/expand-pure-testing-through-venv
branch
from
17510c1 to
124f427
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@internal/cli/commands/catalog/tui/load.go`:
- Line 50: The change to line 50 where `allowCAS := !opts.NoCAS` is set raises
concerns about scope creep and unclear intent within the PR. To address this,
either add clear documentation and tests explaining why this CAS enablement
logic change is necessary and how it relates to the primary PR objectives, or
revert this line if it is not essential to the main changes being made. The
intent of this modification must be transparent to reviewers and maintainers.
In `@internal/cli/commands/catalog/tui/update.go`:
- Around line 635-642: The discoverFormCmd function receives the venv.Venv
parameter v but when calling discoverValuesFields for copyable kinds, it only
passes the component c. The discoverValuesFields function internally hardcodes
vfs.NewOSFS() instead of using the injected v.FS filesystem. To fix this, modify
the discoverValuesFields function signature to accept the venv.Venv parameter v
(similar to how discoverModuleFields receives it), then update the function call
on the copyable branch to pass v, and within discoverValuesFields replace the
hardcoded vfs.NewOSFS() call with v.FS to ensure consistent filesystem handling
across both copyable and module discovery paths.
🪄 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: e4d9f1b5-0916-4d4a-a1f6-b2bd2889aa49
📒 Files selected for processing (24)
internal/cli/commands/catalog/catalog.gointernal/cli/commands/catalog/cli.gointernal/cli/commands/catalog/tui/load.gointernal/cli/commands/catalog/tui/model.gointernal/cli/commands/catalog/tui/model_test.gointernal/cli/commands/catalog/tui/scaffold.gointernal/cli/commands/catalog/tui/update.gointernal/cli/commands/catalog/tui/view_test.gointernal/cli/commands/catalog/tui/welcome.gointernal/cli/commands/catalog/tui/welcome_test.gointernal/cli/commands/commands.gointernal/discovery/graph_target_test.gointernal/runner/run/action_with_hooks_test.gointernal/runner/run/creds/providers/externalcmd/provider_mem_test.gointernal/runner/run/download_source_test.gointernal/runner/run/hook_internal_test.gointernal/runner/run/hook_lifecycle_test.gointernal/runner/run/hook_test.gointernal/runner/run/run_e2e_test.gointernal/runner/run/version_check_mem_test.gointernal/services/catalog/module/repo.gointernal/services/catalog/module/repo_tag_test.gointernal/shell/git_mem_test.gopkg/config/config_helpers_hcl_mem_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/shell/git_mem_test.go
- internal/runner/run/version_check_mem_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/cli/commands/catalog/cli.go
- internal/cli/commands/commands.go
- internal/runner/run/hook_test.go
- internal/runner/run/hook_lifecycle_test.go
- internal/runner/run/download_source_test.go
- internal/runner/run/creds/providers/externalcmd/provider_mem_test.go
- pkg/config/config_helpers_hcl_mem_test.go
- internal/services/catalog/module/repo.go
- internal/runner/run/run_e2e_test.go
- internal/services/catalog/module/repo_tag_test.go
- internal/runner/run/action_with_hooks_test.go
- internal/discovery/graph_target_test.go
- internal/runner/run/hook_internal_test.go
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@internal/cli/commands/catalog/tui/load.go`:
- Line 50: The change to line 50 where `allowCAS := !opts.NoCAS` is set raises
concerns about scope creep and unclear intent within the PR. To address this,
either add clear documentation and tests explaining why this CAS enablement
logic change is necessary and how it relates to the primary PR objectives, or
revert this line if it is not essential to the main changes being made. The
intent of this modification must be transparent to reviewers and maintainers.
In `@internal/cli/commands/catalog/tui/update.go`:
- Around line 635-642: The discoverFormCmd function receives the venv.Venv
parameter v but when calling discoverValuesFields for copyable kinds, it only
passes the component c. The discoverValuesFields function internally hardcodes
vfs.NewOSFS() instead of using the injected v.FS filesystem. To fix this, modify
the discoverValuesFields function signature to accept the venv.Venv parameter v
(similar to how discoverModuleFields receives it), then update the function call
on the copyable branch to pass v, and within discoverValuesFields replace the
hardcoded vfs.NewOSFS() call with v.FS to ensure consistent filesystem handling
across both copyable and module discovery paths.
🪄 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: e4d9f1b5-0916-4d4a-a1f6-b2bd2889aa49
📒 Files selected for processing (24)
internal/cli/commands/catalog/catalog.gointernal/cli/commands/catalog/cli.gointernal/cli/commands/catalog/tui/load.gointernal/cli/commands/catalog/tui/model.gointernal/cli/commands/catalog/tui/model_test.gointernal/cli/commands/catalog/tui/scaffold.gointernal/cli/commands/catalog/tui/update.gointernal/cli/commands/catalog/tui/view_test.gointernal/cli/commands/catalog/tui/welcome.gointernal/cli/commands/catalog/tui/welcome_test.gointernal/cli/commands/commands.gointernal/discovery/graph_target_test.gointernal/runner/run/action_with_hooks_test.gointernal/runner/run/creds/providers/externalcmd/provider_mem_test.gointernal/runner/run/download_source_test.gointernal/runner/run/hook_internal_test.gointernal/runner/run/hook_lifecycle_test.gointernal/runner/run/hook_test.gointernal/runner/run/run_e2e_test.gointernal/runner/run/version_check_mem_test.gointernal/services/catalog/module/repo.gointernal/services/catalog/module/repo_tag_test.gointernal/shell/git_mem_test.gopkg/config/config_helpers_hcl_mem_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/shell/git_mem_test.go
- internal/runner/run/version_check_mem_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/cli/commands/catalog/cli.go
- internal/cli/commands/commands.go
- internal/runner/run/hook_test.go
- internal/runner/run/hook_lifecycle_test.go
- internal/runner/run/download_source_test.go
- internal/runner/run/creds/providers/externalcmd/provider_mem_test.go
- pkg/config/config_helpers_hcl_mem_test.go
- internal/services/catalog/module/repo.go
- internal/runner/run/run_e2e_test.go
- internal/services/catalog/module/repo_tag_test.go
- internal/runner/run/action_with_hooks_test.go
- internal/discovery/graph_target_test.go
- internal/runner/run/hook_internal_test.go
🛑 Comments failed to post (2)
internal/cli/commands/catalog/tui/load.go (1)
50-50:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winClarify the CAS behavior change in Line 50 (UNKNOWN-UNKNOWNS trigger).
Line 50 changes CAS enablement to
allowCAS := !opts.NoCAS, which broadens behavior beyond venv threading. Signature triggered: UNKNOWN-UNKNOWNS (“cannot articulate why this hunk exists from PR intent”). Please either document the intent in this PR (with tests) or move/revert it to a focused change.As per coding guidelines
**/*: “UNKNOWN-UNKNOWNS SCAN… If not, flag the hunk at HIGH and require the maintainer to explain its intent before merge.”🤖 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 `@internal/cli/commands/catalog/tui/load.go` at line 50, The change to line 50 where `allowCAS := !opts.NoCAS` is set raises concerns about scope creep and unclear intent within the PR. To address this, either add clear documentation and tests explaining why this CAS enablement logic change is necessary and how it relates to the primary PR objectives, or revert this line if it is not essential to the main changes being made. The intent of this modification must be transparent to reviewers and maintainers.Source: Coding guidelines
internal/cli/commands/catalog/tui/update.go (1)
635-642:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCopyable form discovery still bypasses injected venv filesystem.
discoverFormCmdthreadsv, butdiscoverValuesFieldsstill hardcodesvfs.NewOSFS()(Line 680). That breaks end-to-end venv consistency for copyable kinds whenv.FSis not OS-backed.Suggested fix
-func discoverFormCmd(ctx context.Context, l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) tea.Cmd { +func discoverFormCmd(ctx context.Context, l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) tea.Cmd { return func() tea.Msg { if c.Kind.IsCopyable() { - return discoverValuesFields(c) + return discoverValuesFields(v, c) } return discoverModuleFields(ctx, l, v, opts, c) } } -func discoverValuesFields(c *Component) tea.Msg { +func discoverValuesFields(v venv.Venv, c *Component) tea.Msg { configName := configFileForKind(c.Kind) if configName == "" { return formDiscoveryErrMsg{err: fmt.Errorf("component kind %q has no associated HCL file", c.Kind)} } - refs, err := CollectValuesReferences(vfs.NewOSFS(), filepath.Join(c.Repo.Path(), c.Dir, configName)) + refs, err := CollectValuesReferences(v.FS, filepath.Join(c.Repo.Path(), c.Dir, configName)) if err != nil { return formDiscoveryErrMsg{err: err} }Also applies to: 674-683
🤖 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 `@internal/cli/commands/catalog/tui/update.go` around lines 635 - 642, The discoverFormCmd function receives the venv.Venv parameter v but when calling discoverValuesFields for copyable kinds, it only passes the component c. The discoverValuesFields function internally hardcodes vfs.NewOSFS() instead of using the injected v.FS filesystem. To fix this, modify the discoverValuesFields function signature to accept the venv.Venv parameter v (similar to how discoverModuleFields receives it), then update the function call on the copyable branch to pass v, and within discoverValuesFields replace the hardcoded vfs.NewOSFS() call with v.FS to ensure consistent filesystem handling across both copyable and module discovery paths.
denis256
approved these changes
Jun 18, 2026
| // TODO: thread venv from the CLI entrypoint through the catalog TUI | ||
| // so this leaf participates in the root virtualized environment. | ||
| return scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts, c.component.TerraformSourcePath(), "") | ||
| return scaffold.Run(context.Background(), c.logger, c.venv, c.opts, c.component.TerraformSourcePath(), "") |
Collaborator
Author
There was a problem hiding this comment.
Ya, I'll get to that later.
| return formDiscoveryErrMsg{err: fmt.Errorf("component kind %q has no associated HCL file", c.Kind)} | ||
| } | ||
|
|
||
| refs, err := CollectValuesReferences(vfs.NewOSFS(), filepath.Join(c.Repo.Path(), c.Dir, configName)) |
Member
There was a problem hiding this comment.
New VFS instance, should pass the Venv instance/field as an argument?
| // copyComponentWithValuesCmd schedules the unit/stack copy with the form's | ||
| // collected HCL values threaded through CopyCmd.WithValues. | ||
| func copyComponentWithValuesCmd(l log.Logger, m Model, c *Component, values map[string]string) tea.Cmd { | ||
| cmd := NewCopyCmd(l, m.terragruntOptions, c).WithValues(values) |
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.
Description
Expands the pure testing we can do throughout the codebase using venv.
TODOs
Read the Gruntwork contribution guidelines.
Summary by CodeRabbit
Tests
Chores