feat(local): add colima resize, cloudflare bootstrap, aws region/runtime#68
Merged
Conversation
📝 WalkthroughWalkthroughRemoves the shared ChangesLocal Dev Subcommands, Per-Provider DRC Split, and Doctor
Sequence Diagram(s)sequenceDiagram
participant CLI as hops local cloudflare
participant resolver as resolve_api_token
participant aws as AWS CLI (secretsmanager)
participant kubectl as kubectl apply
CLI->>resolver: resolve token (--api-token / CLOUDFLARE_API_TOKEN / AWS secret)
resolver->>aws: get-secret-value --secret-id <id>
aws-->>resolver: SecretString
alt SSO error detected
resolver->>aws: aws sso login --profile <profile>
aws-->>resolver: ok
resolver->>aws: get-secret-value (retry)
aws-->>resolver: SecretString
end
resolver-->>CLI: api_token
CLI->>kubectl: apply Provider YAML
CLI->>kubectl: wait for CRDs (providerconfig, dnsrecord)
CLI->>kubectl: apply Secret YAML
CLI->>kubectl: apply ProviderConfig YAML
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
patrickleet
force-pushed
the
feat/local-listmonk-bootstrap
branch
from
34d2fda to
6fbbb96
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/commands/local/aws.rs`:
- Line 52: The refresh path in aws.rs applies the DRC but exits before updating
the Provider, so existing providers can keep a stale AWS region. Update the
refresh flow in the relevant command handler to also wire the Provider to the
refreshed runtime config by patching or reconciling the Provider when its
runtimeConfigRef.name does not match args.runtime_config_name. Use the existing
refresh/apply logic in the command implementation to ensure the Provider is
brought in sync before returning success.
In `@src/commands/local/doctor.rs`:
- Around line 170-181: The ClusterRoleBinding validation in ProviderExpectation
only checks ServiceAccount names, so a matching SA in the wrong namespace can
still pass. Update ProviderExpectation to carry the expected subject namespace,
then adjust the doctor validation around jsonpath and binding_ok to verify both
the ServiceAccount name and namespace for the binding identified by e.binding
and e.sa.
- Around line 238-251: `exists()` and `jsonpath()` currently swallow all
`kubectl` failures into `false`/`None`, so `doctor` can misreport
connectivity/auth problems as missing resources. Update the `doctor` flow in
`src/commands/local/doctor.rs` to perform an upfront cluster-access check before
calling these helpers, or have `exists`/`jsonpath` preserve the first `kubectl`
error and surface it in the report. Use the existing `run_cmd_output`, `exists`,
and `jsonpath` paths to locate the affected logic and ensure broken
kube-context/API failures are shown explicitly instead of drift.
In `@src/commands/local/start.rs`:
- Around line 80-89: The Colima instance selection logic is using the first JSON
item from the list output instead of the default profile, which can cause resize
and running-state checks to target the wrong VM when multiple profiles exist.
Update the code that processes the Colima array output in the local start flow
to identify and select the instance marked as the default profile, and make sure
the subsequent validation and start/stop checks use that chosen instance
consistently. Use the existing ColimaInstance type and the list/selection logic
around the Colima start/stop handling to locate the fix.
- Around line 307-323: The validation in validate_requested_size only checks for
disk shrinking, but it should also reject zero-sized resources before Colima is
invoked. Update this function to validate ColimaSizeArgs fields for --cpus,
--memory, and --disk, returning an error when any requested value is 0,
alongside the existing ColimaInstance::disk_gib shrink check. Keep the logic
centralized here so the start flow fails early with a clear message instead of
reaching the external command path.
In `@src/commands/local/zitadel.rs`:
- Around line 34-37: `ZitadelCommandArgs::source_context` is currently required
by clap for every invocation, even when `--access-token` or
`ZITADEL_ACCESS_TOKEN` is used and the source secret is never read. Make
`source_context` optional in the argument struct and move the requirement into
the source-secret fallback path in `src/commands/local/zitadel.rs`, so only the
code path that actually loads the secret enforces it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ca3dc82-8faf-4a10-9ece-636db6c36f4f
📒 Files selected for processing (20)
bootstrap/drc/helm.yamlbootstrap/drc/kubernetes.yamlbootstrap/drc/local-dev.yamlbootstrap/providers/provider-helm.yamlbootstrap/providers/provider-kubernetes.yamlsrc/commands/auth/bootstrap.rssrc/commands/config/install.rssrc/commands/local/aws.rssrc/commands/local/cloudflare.rssrc/commands/local/doctor.rssrc/commands/local/listmonk.rssrc/commands/local/mod.rssrc/commands/local/package_install.rssrc/commands/local/resize.rssrc/commands/local/start.rssrc/commands/local/zitadel.rssrc/commands/provider/install.rssrc/commands/secrets/list.rssrc/commands/vars/mod.rssrc/commands/vars/sync.rs
💤 Files with no reviewable changes (1)
- bootstrap/drc/local-dev.yaml
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: 6
🤖 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 `@src/commands/local/aws.rs`:
- Line 52: The refresh path in aws.rs applies the DRC but exits before updating
the Provider, so existing providers can keep a stale AWS region. Update the
refresh flow in the relevant command handler to also wire the Provider to the
refreshed runtime config by patching or reconciling the Provider when its
runtimeConfigRef.name does not match args.runtime_config_name. Use the existing
refresh/apply logic in the command implementation to ensure the Provider is
brought in sync before returning success.
In `@src/commands/local/doctor.rs`:
- Around line 170-181: The ClusterRoleBinding validation in ProviderExpectation
only checks ServiceAccount names, so a matching SA in the wrong namespace can
still pass. Update ProviderExpectation to carry the expected subject namespace,
then adjust the doctor validation around jsonpath and binding_ok to verify both
the ServiceAccount name and namespace for the binding identified by e.binding
and e.sa.
- Around line 238-251: `exists()` and `jsonpath()` currently swallow all
`kubectl` failures into `false`/`None`, so `doctor` can misreport
connectivity/auth problems as missing resources. Update the `doctor` flow in
`src/commands/local/doctor.rs` to perform an upfront cluster-access check before
calling these helpers, or have `exists`/`jsonpath` preserve the first `kubectl`
error and surface it in the report. Use the existing `run_cmd_output`, `exists`,
and `jsonpath` paths to locate the affected logic and ensure broken
kube-context/API failures are shown explicitly instead of drift.
In `@src/commands/local/start.rs`:
- Around line 80-89: The Colima instance selection logic is using the first JSON
item from the list output instead of the default profile, which can cause resize
and running-state checks to target the wrong VM when multiple profiles exist.
Update the code that processes the Colima array output in the local start flow
to identify and select the instance marked as the default profile, and make sure
the subsequent validation and start/stop checks use that chosen instance
consistently. Use the existing ColimaInstance type and the list/selection logic
around the Colima start/stop handling to locate the fix.
- Around line 307-323: The validation in validate_requested_size only checks for
disk shrinking, but it should also reject zero-sized resources before Colima is
invoked. Update this function to validate ColimaSizeArgs fields for --cpus,
--memory, and --disk, returning an error when any requested value is 0,
alongside the existing ColimaInstance::disk_gib shrink check. Keep the logic
centralized here so the start flow fails early with a clear message instead of
reaching the external command path.
In `@src/commands/local/zitadel.rs`:
- Around line 34-37: `ZitadelCommandArgs::source_context` is currently required
by clap for every invocation, even when `--access-token` or
`ZITADEL_ACCESS_TOKEN` is used and the source secret is never read. Make
`source_context` optional in the argument struct and move the requirement into
the source-secret fallback path in `src/commands/local/zitadel.rs`, so only the
code path that actually loads the secret enforces it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ca3dc82-8faf-4a10-9ece-636db6c36f4f
📒 Files selected for processing (20)
bootstrap/drc/helm.yamlbootstrap/drc/kubernetes.yamlbootstrap/drc/local-dev.yamlbootstrap/providers/provider-helm.yamlbootstrap/providers/provider-kubernetes.yamlsrc/commands/auth/bootstrap.rssrc/commands/config/install.rssrc/commands/local/aws.rssrc/commands/local/cloudflare.rssrc/commands/local/doctor.rssrc/commands/local/listmonk.rssrc/commands/local/mod.rssrc/commands/local/package_install.rssrc/commands/local/resize.rssrc/commands/local/start.rssrc/commands/local/zitadel.rssrc/commands/provider/install.rssrc/commands/secrets/list.rssrc/commands/vars/mod.rssrc/commands/vars/sync.rs
💤 Files with no reviewable changes (1)
- bootstrap/drc/local-dev.yaml
🛑 Comments failed to post (6)
src/commands/local/aws.rs (1)
52-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure refresh wires the Provider to the refreshed runtime config.
The refresh path applies the DRC but returns before applying or patching the Provider. Existing providers without
runtimeConfigRef.name == args.runtime_config_namewill not receive the refreshed AWS region despite the success log.🐛 Proposed fix
if args.refresh { log::info!( "Applying AWS provider runtime '{}' for region '{}'...", args.runtime_config_name, region ); kubectl_apply_stdin(&build_runtime_config_yaml( &args.runtime_config_name, ®ion, ))?; + + log::info!( + "Ensuring provider-family-aws uses runtime '{}'...", + args.runtime_config_name + ); + kubectl_apply_stdin(&build_provider_yaml( + &args.provider_name, + &args.provider_package, + &args.runtime_config_name, + ))?; log::info!( "Refreshing secret '{}/{}' with generated credentials...",Also applies to: 75-84
🤖 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 `@src/commands/local/aws.rs` at line 52, The refresh path in aws.rs applies the DRC but exits before updating the Provider, so existing providers can keep a stale AWS region. Update the refresh flow in the relevant command handler to also wire the Provider to the refreshed runtime config by patching or reconciling the Provider when its runtimeConfigRef.name does not match args.runtime_config_name. Use the existing refresh/apply logic in the command implementation to ensure the Provider is brought in sync before returning success.src/commands/local/doctor.rs (2)
170-181: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the ClusterRoleBinding subject namespace too.
Line 175 only checks ServiceAccount names. A binding to the same SA name in the wrong namespace will still pass here even though the provider will not get the pinned identity. Add the expected subject namespace to
ProviderExpectationand verify both fields.🤖 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 `@src/commands/local/doctor.rs` around lines 170 - 181, The ClusterRoleBinding validation in ProviderExpectation only checks ServiceAccount names, so a matching SA in the wrong namespace can still pass. Update ProviderExpectation to carry the expected subject namespace, then adjust the doctor validation around jsonpath and binding_ok to verify both the ServiceAccount name and namespace for the binding identified by e.binding and e.sa.
238-251: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Surface kube-context/API failures before reporting drift.
exists()andjsonpath()turn everykubectlerror intofalse/None. If the selected context is broken or the API is unreachable,doctorwill report a pile of missing resources and suggest rerunninghops local startinstead of showing the real connectivity/auth failure. Add an upfront cluster-access check or preserve the firstkubectlerror in the report.🤖 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 `@src/commands/local/doctor.rs` around lines 238 - 251, `exists()` and `jsonpath()` currently swallow all `kubectl` failures into `false`/`None`, so `doctor` can misreport connectivity/auth problems as missing resources. Update the `doctor` flow in `src/commands/local/doctor.rs` to perform an upfront cluster-access check before calling these helpers, or have `exists`/`jsonpath` preserve the first `kubectl` error and surface it in the report. Use the existing `run_cmd_output`, `exists`, and `jsonpath` paths to locate the affected logic and ensure broken kube-context/API failures are shown explicitly instead of drift.src/commands/local/start.rs (2)
80-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Select the default Colima profile instead of the first JSON item.
For array output, Line 373 picks the first instance, but subsequent
colima start/stopcalls target the default profile. With multiple Colima profiles, resize validation and running-state checks can use the wrong VM.🐛 Proposed fix
#[derive(Debug, Clone, Deserialize, PartialEq, Eq)] pub(crate) struct ColimaInstance { + #[serde(default)] + name: String, #[serde(default)] status: String, @@ - if let Ok(instances) = serde_json::from_str::<Vec<ColimaInstance>>(trimmed) { - return Ok(instances.into_iter().next()); + if let Ok(instances) = serde_json::from_str::<Vec<ColimaInstance>>(trimmed) { + return Ok(select_default_colima_instance(instances)); } @@ } + +fn select_default_colima_instance(instances: Vec<ColimaInstance>) -> Option<ColimaInstance> { + let mut first = None; + for instance in instances { + if instance.name == "default" { + return Some(instance); + } + if first.is_none() { + first = Some(instance); + } + } + first +}Also applies to: 362-374
🤖 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 `@src/commands/local/start.rs` around lines 80 - 89, The Colima instance selection logic is using the first JSON item from the list output instead of the default profile, which can cause resize and running-state checks to target the wrong VM when multiple profiles exist. Update the code that processes the Colima array output in the local start flow to identify and select the instance marked as the default profile, and make sure the subsequent validation and start/stop checks use that chosen instance consistently. Use the existing ColimaInstance type and the list/selection logic around the Colima start/stop handling to locate the fix.
307-323: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject zero-sized Colima resources before invoking Colima.
--cpus 0,--memory 0, and--disk 0currently pass parsing and fail later in the external command path. Validate them alongside the disk-shrink check.🐛 Proposed fix
fn validate_requested_size( size: &ColimaSizeArgs, instance: Option<&ColimaInstance>, ) -> Result<(), Box<dyn Error>> { + if size.cpus == Some(0) { + return Err("Colima CPUs must be greater than 0".into()); + } + if size.memory == Some(0) { + return Err("Colima memory must be greater than 0GiB".into()); + } + if size.disk == Some(0) { + return Err("Colima disk must be greater than 0GiB".into()); + } + if let (Some(requested), Some(current)) = (size.disk, instance.and_then(ColimaInstance::disk_gib)) {📝 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.fn validate_requested_size( size: &ColimaSizeArgs, instance: Option<&ColimaInstance>, ) -> Result<(), Box<dyn Error>> { if size.cpus == Some(0) { return Err("Colima CPUs must be greater than 0".into()); } if size.memory == Some(0) { return Err("Colima memory must be greater than 0GiB".into()); } if size.disk == Some(0) { return Err("Colima disk must be greater than 0GiB".into()); } if let (Some(requested), Some(current)) = (size.disk, instance.and_then(ColimaInstance::disk_gib)) { if requested < current { return Err(format!( "Colima disk cannot be shrunk from {current}GiB to {requested}GiB. Use --disk {current} or larger, or destroy and recreate the VM." ) .into()); } } Ok(()) }🤖 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 `@src/commands/local/start.rs` around lines 307 - 323, The validation in validate_requested_size only checks for disk shrinking, but it should also reject zero-sized resources before Colima is invoked. Update this function to validate ColimaSizeArgs fields for --cpus, --memory, and --disk, returning an error when any requested value is 0, alongside the existing ColimaInstance::disk_gib shrink check. Keep the logic centralized here so the start flow fails early with a clear message instead of reaching the external command path.src/commands/local/zitadel.rs (1)
34-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline src/commands/local/zitadel.rs --view expanded rg -n -C3 'access_token|ZITADEL_ACCESS_TOKEN|source_context|source secret|source cluster' src/commands/local/zitadel.rsRepository: hops-ops/hops-cli
Length of output: 4451
Make
--source-contextconditional on the source-secret fallback path.source_contextis required by clap on every invocation here, so--access-token ...andZITADEL_ACCESS_TOKENcallers still have to pass a source cluster context even though those paths never read the secret. Keep it optional at parse time and require it only when falling back to the source secret.🤖 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 `@src/commands/local/zitadel.rs` around lines 34 - 37, `ZitadelCommandArgs::source_context` is currently required by clap for every invocation, even when `--access-token` or `ZITADEL_ACCESS_TOKEN` is used and the source secret is never read. Make `source_context` optional in the argument struct and move the requirement into the source-secret fallback path in `src/commands/local/zitadel.rs`, so only the code path that actually loads the secret enforces it.
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
1 participant
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.
Summary
hops local startnow accepts--cpus(alias--cpu),--memory,--disk+--yesfor non-interactive resize on existing VM.hops local resize --cpus N --memory N --disk Nfor non-destructive Colima VM resize (detects current viacolima list --json, prevents disk shrink).hops local cloudflare(token from flag / env / AWS SM; applies provider + creds secret + ProviderConfig; --refresh support). Mirrors aws/zitadel/listmonk/github.hops local awsgains--region,--runtime-config-name; always ensures region-aware DeploymentRuntimeConfig (normal + refresh paths).Branch rebased onto latest
mainfor up-to-date base (previously based on stale feat branch).Verification
cargo test: 104 passedcargo checkcleanTasks (GitKB)
Commits (on feat/local-listmonk-bootstrap, rebased)