ROX-35667: configurable user config path via ROXIE_USER_CONFIG_PATH#233
Conversation
Allow overriding the default user config file path by setting the ROXIE_USER_CONFIG_PATH environment variable. When set to a non-empty value, UserConfigPath() returns it directly instead of the default $CONFIG_DIR/roxie/config.yaml path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesUser config path env override
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 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/paths/paths_test.go`:
- Around line 21-24: The "returns default path when env var is not set" subtest
in paths_test.go currently leaves ROXIE_USER_CONFIG_PATH uncontrolled when
setEnv is false, so the test can depend on ambient environment state. Update the
test setup around the UserConfigPath subtests to always call t.Setenv explicitly
for ROXIE_USER_CONFIG_PATH, using an empty string for the default-path case and
a real value for the override case, so the fallback behavior is exercised
deterministically.
🪄 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: e18a3831-9bb1-45a1-90de-8f4b85514d12
📒 Files selected for processing (2)
internal/paths/paths.gointernal/paths/paths_test.go
There was a problem hiding this comment.
From the code perspective: this looks good!
From how this works perspective:
Is the path given by the user being validated?
I tried:
➜ ROXIE_USER_CONFIG_PATH='fckgw-rhqq2 $$!~!!!' roxie deploy --dry-run
00:00 Running with a controlling terminal.
00:00 Looking up main image tag
00:00 No MAIN_IMAGE_TAG found in the environment, using default main image tag 4.9.2 for deployment
00:00 To use a different tag, set the MAIN_IMAGE_TAG environment variable
00:00 Alternatively, execute roxie from within the stackrox repository, in which case the currently checked out stackrox tag will be used
00:00 Detected cluster type: Unknown
00:00 Detected container runtime socket: unix:///var/run/docker.sock
00:00 🚀 ACS Deployer initialized
00:00 Exiting because of enabled dry run mode.
but maybe the dry-run mode is not covering this?
Also help text is not being updated:
➜ ROXIE_USER_CONFIG_PATH="~/.roxie.yaml" roxie deploy -h
Deploy ACS components (central, secured-cluster, operator).
(...)
Global Flags:
--dry-run Do not actually modify cluster
--skip-user-config Skips reading of user's configuration (/Users/prygiels/Library/Application Support/roxie/config.yaml)
...
Putting here a bogus value without dry-run started tearing down @porridge 's cluster without any checks or warnings (sorry):
➜ ROXIE_USER_CONFIG_PATH='fckgw-rhqq2 $$!~!!!' roxie deploy both
00:00 Using kubeconfig ~/.cluster2/kubeconfig
00:02 Running with a controlling terminal.
00:02 Looking up main image tag
00:02 No MAIN_IMAGE_TAG found in the environment, using default main image tag 4.9.2 for deployment
00:02 To use a different tag, set the MAIN_IMAGE_TAG environment variable
00:02 Alternatively, execute roxie from within the stackrox repository, in which case the currently checked out stackrox tag will be used
00:02 Detected cluster type: GKE (infra)
00:02 Detected container runtime socket: unix:///var/run/docker.sock
00:02 🚀 ACS Deployer initialized
00:02
00:02 ┌────────────────────────────────────────────────────────────┐
00:02 │ Deployment Configuration │
00:02 ├────────────────────────────────────────────────────────────┤
00:02 │ Component: Central │
00:02 │ Cluster Type: GKE (infra) │
00:02 │ Main Tag: 4.9.2 │
00:02 │ Kubernetes Context: gke_acs-team-temp-dev_us-centra... │
00:02 │ Exposure: loadbalancer │
00:02 └────────────────────────────────────────────────────────────┘
00:02
00:02
00:02 ┌────────────────────────────────────────────────────────────┐
00:02 │ Deployment Configuration │
00:02 ├────────────────────────────────────────────────────────────┤
00:02 │ Component: Secured Cluster │
00:02 │ Cluster Type: GKE (infra) │
00:02 │ Main Tag: 4.9.2 │
00:02 │ Kubernetes Context: gke_acs-team-temp-dev_us-centra... │
00:02 └────────────────────────────────────────────────────────────┘
00:02
00:02 Preparing and verifying Docker credentials...
00:02 Successfully verified credentials for quay.io (repository: rhacs-eng/main)
00:02 Docker credentials verified successfully
00:02 Initiating deployment of Central and Secured Cluster
00:06 Operator version mismatch detected:
00:06 Current: 4.11.0
00:06 Desired: 4.9.2
00:06 🔄 Operator version mismatch, redeploying...
00:06 🧹 Tearing down operator deployed without OLM...
^C%
|
@vikin91 Are you sure that you have the correct roxie in PATH? 🤔 |
|
And for your other question, @vikin91 , this is NOT being validated. |
|
@vikin91, as for this example from you: ❮ ROXIE_USER_CONFIG_PATH=~/.roxie.yaml ./roxie --help | rg configuration ❯ ROXIE_USER_CONFIG_PATH="~/.roxie.yaml" ./roxie --help | rg configuration |
mclasmeier
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/paths/paths.go (1)
15-15: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
os.Openoveros.ReadFilefor readability checks.
os.ReadFilereads the entire file contents into memory only to discard them. If the intent is to verify existence and read permission,os.Openfollowed by an immediateCloseis more appropriate and avoids unnecessary I/O for large files.♻️ Proposed refactor
- if _, err := os.ReadFile(overwrittenPath); err != nil { + f, err := os.Open(overwrittenPath) + if err != nil { return "", fmt.Errorf("config file %q specified in environment variable %s cannot be read: %w", overwrittenPath, envUserConfigPath, err) } + _ = f.Close() return overwrittenPath, nil🤖 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/paths/paths.go` at line 15, The readability check in the overwritten path handling is using os.ReadFile just to confirm the file can be accessed, which unnecessarily loads the file contents. Update the logic around the overwrittenPath check to use os.Open followed by an immediate Close instead, keeping the same error handling in the relevant path helper that performs the existence/read-permission check.
🤖 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/paths/paths.go`:
- Around line 14-19: The new readability check in the env override branch of
user config path resolution will break existing tests because they set
ROXIE_USER_CONFIG_PATH to a nonexistent file and still expect success. Update
the tests around user config path handling, especially the env-override cases in
paths_test.go and the UserConfigPathString path, to use a real temporary file
created with t.TempDir() when asserting success, and add a separate test that
explicitly covers the unreadable/nonexistent-path error returned by the
UserConfigPath logic.
---
Nitpick comments:
In `@internal/paths/paths.go`:
- Line 15: The readability check in the overwritten path handling is using
os.ReadFile just to confirm the file can be accessed, which unnecessarily loads
the file contents. Update the logic around the overwrittenPath check to use
os.Open followed by an immediate Close instead, keeping the same error handling
in the relevant path helper that performs the existence/read-permission check.
🪄 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: d3c3e13a-0130-4ba3-8783-2c744213cc09
📒 Files selected for processing (1)
internal/paths/paths.go
vikin91
left a comment
vikin91
left a comment
There was a problem hiding this comment.
LGTM provided that the unit-tests are fixed (and they don't change behavior).
Excerpt from manual tests:
$ ROXIE_USER_CONFIG_PATH='fckgw-rhqq2 $$!~!!!' ./roxie deploy both
00:00 Using kubeconfig /Users/prygiels/.cluster2/kubeconfig
00:02 Running with a controlling terminal.
Error: applying user config: config file "fckgw-rhqq2 $$!~!!!" specified in environment variable ROXIE_USER_CONFIG_PATH cannot be read: open fckgw-rhqq2 $$!~!!!: no such file or directory
$ ROXIE_USER_CONFIG_PATH='/tmp/not-there' ./roxie deploy both -c "/tmp/not-there-2"
Error: invalid argument "/tmp/not-there-2" for "-c, --config" flag: failed to read config file "/tmp/not-there-2": open /tmp/not-there-2: no such file or directory
$ ROXIE_USER_CONFIG_PATH='/tmp/not-there' ./roxie deploy both
00:00 Using kubeconfig /Users/prygiels/.cluster2/kubeconfig
00:00 Running with a controlling terminal.
Error: applying user config: config file "/tmp/not-there" specified in environment variable ROXIE_USER_CONFIG_PATH cannot be read: open /tmp/not-there: no such file or directory
$ ROXIE_USER_CONFIG_PATH='/tmp/not-there' ./roxie deploy both --dry-run
00:00 Running with a controlling terminal.
Error: applying user config: config file "/tmp/not-there" specified in environment variable ROXIE_USER_CONFIG_PATH cannot be read: open /tmp/not-there: no such file or directory
…ould still the path in --help, even if it doesn't exist. Updated tests and small refactoring
mclasmeier
force-pushed
the
configurable-user-config-path
branch
from
7b0b383 to
e03f7ad
Compare
|
@coderabbitai, fresh review please. |
|
✅ Action performedFull review finished. |
|
@vikin91 There was a small issue left in the version you have approved: So, what I had to do is, smuggle a Accompanied with that are some minor refactoring in |
|
Got it! Still green for me! Passing a bool into a function may suggest that the function might need splitting into two functions 😉 |
2 participants
Add configurable user config path via ROXIE_USER_CONFIG_PATH
Set
ROXIE_USER_CONFIG_PATHto override the default user config file location ($CONFIG_DIR/roxie/config.yaml). When unset or empty, existing behavior is preserved.Changes:
internal/paths/paths.go— check env var at top ofUserConfigPath()internal/paths/paths_test.go— new, 5 test casesSummary by CodeRabbit
New Features
Tests
Bug Fixes