Implement 'roxie shell' command for (re-)connecting to central deployment#201
Implement 'roxie shell' command for (re-)connecting to central deployment#201mclasmeier wants to merge 15 commits into
Conversation
Simplify internal/types/roxie_environment.go
|
Warning Review limit reached
More reviews will be available in 56 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds persistent ROXIE manifest storage in a Kubernetes Secret, new types and helpers for assembling a ROXIE environment from central deployment info, CLI integration to create/load/delete the manifest (deploy/teardown), and a new shell command/subshell refactor that uses the assembled environment. ChangesROXIE Manifest Persistence and Environment Management Feature
Sequence DiagramsequenceDiagram
participant user as User
participant deployCmd as Deploy Command
participant deployer as Deployer
participant manifest as manifest package
participant kubectl as kubectl
participant shellCmd as Shell Command
participant subshell as Subshell
user->>deployCmd: roxie deploy
deployCmd->>deployer: WaitForCentral()
deployer-->>deployCmd: CentralDeploymentInfo
deployCmd->>manifest: CreateManifestSecretOnCluster(RoxieManifest)
manifest->>kubectl: apply namespace & apply Secret (stdin)
kubectl->>manifest: cluster applies Secret
deployCmd-->>user: continue (warnings logged if failure)
user->>shellCmd: roxie shell
shellCmd->>manifest: LoadManifestSecret()
manifest->>kubectl: get Secret data
kubectl->>manifest: return Secret data
manifest-->>shellCmd: RoxieManifest -> CentralDeploymentInfo (attempt CA fetch)
shellCmd->>subshell: runCommandOrSubshell(info, args)
subshell->>subshell: Assemble env via AssembleRoxieEnvironment(...).Export()
subshell->>subshell: optionally start HAProxy
subshell-->>user: launch interactive shell or run command
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/deploy.go (1)
342-351: ⚡ Quick winConfirm
manifestAPI matches thecmd/deploy.gocall site; drop the type/signature mismatch concern.
internal/manifest/manifest.godefinesRoxieManifestwithRoxieEnvironment types.RoxieEnvironmentandConfig deployer.Config, andCreateManifestSecretOnCluster(ctx, log, m RoxieManifest) error, matching the constructedmand call signature incmd/deploy.go. If CI still typechecks-fails, the issue is elsewhere.
- If
dryRunalready returns earlier incmd/deploy.go, the&& !dryRunguard around this block can be removed.🤖 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 `@cmd/deploy.go` around lines 342 - 351, The call site constructs a manifest.RoxieManifest using roxieenv.AssembleRoxieEnvironment(d.GetCentralDeploymentInfo()) and deploySettings and passes it to manifest.CreateManifestSecretOnCluster(ctx, log, m); confirm the manifest.RoxieManifest type and CreateManifestSecretOnCluster(ctx, log, m RoxieManifest) signature in internal/manifest/manifest.go match exactly (field names/types for RoxieEnvironment and Config) and fix any mismatched imports or type aliases; also, if cmd/deploy.go already returns early on dryRun, remove the redundant "&& !dryRun" guard around the block guarded by components.IncludesCentral() to avoid dead/duplicate checks.
🤖 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 `@cmd/subshell.go`:
- Around line 54-89: The command mode currently wraps any child non-zero exit
into a generic error instead of propagating the child's exit code; change the
error handling after cmd.Run() so that when err is an *exec.ExitError you
extract the exit status (use exitErr.ExitCode()) and call os.Exit(exitCode) (or
otherwise return a sentinel that the caller converts into that exit code),
rather than returning fmt.Errorf("failed to execute command: %w", err); keep the
existing behavior for non-start failures. Locate this logic around the cmd.Run()
call and the error handling for both the subshell branch and the non-subshell
branch (referencing subShellMode, cmd.Run, and exec.ExitError) and implement the
exit-code propagation for the non-subshell command path.
---
Nitpick comments:
In `@cmd/deploy.go`:
- Around line 342-351: The call site constructs a manifest.RoxieManifest using
roxieenv.AssembleRoxieEnvironment(d.GetCentralDeploymentInfo()) and
deploySettings and passes it to manifest.CreateManifestSecretOnCluster(ctx, log,
m); confirm the manifest.RoxieManifest type and
CreateManifestSecretOnCluster(ctx, log, m RoxieManifest) signature in
internal/manifest/manifest.go match exactly (field names/types for
RoxieEnvironment and Config) and fix any mismatched imports or type aliases;
also, if cmd/deploy.go already returns early on dryRun, remove the redundant "&&
!dryRun" guard around the block guarded by components.IncludesCentral() to avoid
dead/duplicate checks.
🪄 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.yml
Review profile: CHILL
Plan: Enterprise
Run ID: c33d0da6-23cf-44f4-b552-8bfff8724aa8
📒 Files selected for processing (11)
cmd/deploy.gocmd/main.gocmd/subshell.gocmd/teardown.gointernal/deployer/deployer.gointernal/manifest/manifest.gointernal/manifest/manifest_integration_test.gointernal/roxieenv/env.gointernal/roxieenv/env_test.gointernal/types/central_deployment_info.gointernal/types/roxie_environment.go
…ironment Provide separate function for spawning subshell given a deployer.
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 `@cmd/shell.go`:
- Around line 31-34: Fix the typo in the examples block by replacing the
incorrect "roxie shel" string with the correct "roxie shell" so the advertised
primary usage matches the actual command; locate the Examples/usage string where
"roxie shel" appears and update that literal to "roxie shell".
- Around line 35-42: The Run function currently exits with os.Exit(1) for
non-*exec.ExitError errors without logging; update the Run closure (the
anonymous func calling runShell) so that when err != nil and it is not an
*exec.ExitError you first print the error (e.g., fmt.Fprintln(os.Stderr, err) or
use a logger) before calling os.Exit(1). Locate the error handling block that
uses errors.AsType[*exec.ExitError] and add a branch to output the error to
stderr, preserving the existing exit behavior for child process exit errors
returned by runShell.
🪄 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.yml
Review profile: CHILL
Plan: Enterprise
Run ID: f54999d9-3ae8-480f-937f-2f05a70bb3b2
📒 Files selected for processing (2)
cmd/shell.gocmd/subshell.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/subshell.go
| type roxieEnvironmentStd RoxieEnvironment | ||
|
|
||
| // MarshalYAML skips local-only fields (ROX_CA_CERT_FILE). | ||
| func (r RoxieEnvironment) MarshalYAML() (any, error) { |
There was a problem hiding this comment.
Nitpick - it might be nice to add a quick unit test that marshals a RoxieEnvironment with RoxCaCertFile set and asserts the output YAML doesn't contain ROX_CA_CERT_FILE.
|
Approved with one minor comment, great work! |
2 participants
This addresses #197 but in a more general way.
Adds a
roxie shellcommand that opens a subshell (or runs a command) against an existing ACS deployment without re-deploying.How it works:
roxie deploynow persists a manifest secret (roxie-manifest) in a dedicatedroxienamespace on the cluster.roxie shellreads that secret, re-fetches the CA certificate, and spawns a subshell with the same environment variables (ROX_ENDPOINT,ROX_ADMIN_PASSWORD, etc.) thatroxie deploysets up.Changes:
roxie shellcommand (cmd/shell.go)manifestpackage — stores/loads/deletes aRoxieManifestsecret on-cluster, including deploy config for reproducibilityroxieenvpackage — single source of truth for assembling the ACS environment variables (used by deploy, envrc, and shell)CentralDeploymentInfoandRoxieEnvironmenttypes extracted tointernal/typesRoxieEnvironmentcustom YAML marshaler excludes local-only fields (ROX_CA_CERT_FILE)roxienamespace--dry-runSummary by CodeRabbit
New Features
Improvements
Tests