feat(rest-api): add reloadable phone home URL settings#2289
Conversation
Signed-off-by: Hasan Khan <hasank@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbitRelease Notes
WalkthroughThis PR introduces runtime settings management with site config overlay capability, automatic file-watching reloads, and a new metadata API endpoint. CLI argument preprocessing routes config file paths via environment variables before configuration initialization. The Config struct is extended with a RuntimeSettings snapshot guarded by RWMutex, supporting both base and site overlay merging. ReloadConfig and WatchConfigFilePaths enable hot-reloading of configuration changes. A new metadata settings endpoint exposes runtime settings as JSON. ChangesRuntime Settings and Metadata API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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: 2
🧹 Nitpick comments (2)
rest-api/api/cmd/api/main_test.go (1)
15-28: ⚡ Quick winAdd parser branch coverage for inline and malformed args.
This test covers only one happy path. Please add cases for
--config-path=<value>and missing-value forms (for both flags) so regressions in the manual parser are caught early.🤖 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 `@rest-api/api/cmd/api/main_test.go` around lines 15 - 28, Add unit tests inside TestApplyConfigPathArgs (or new subtests) to cover the parser branches for inline flag syntax and missing-value errors: call applyConfigPathArgs with args using "--config-path=/path" and "--site-config-path=/path" and assert the corresponding env vars (config.ConfigFilePathEnv, config.ConfigSiteConfigFilePathEnv) are set; also call applyConfigPathArgs with malformed/missing-value forms like "--config-path" (end of slice) and "--site-config-path=" (empty value) and assert the env vars remain unchanged or are cleared as the parser should behave, and add asserts for expected behavior (error/empty) for each case to prevent regressions in applyConfigPathArgs.rest-api/api/internal/config/config.go (1)
1338-1343: ⚡ Quick winFilter fsnotify events to the configured files only.
At Line 1338, every write/create/rename in the watched directories triggers a reload. In busy directories this causes avoidable reload churn and log noise. Restrict reload triggers to
configFilePathandsiteConfigFilePathevent targets.🤖 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 `@rest-api/api/internal/config/config.go` around lines 1338 - 1343, The watcher currently reloads on any Write/Create/Rename event; restrict this to only the configured config files by checking the event target name before calling c.ReloadConfig(). Inside the loop where event := <-watcher.Events is handled (the block that calls event.Has and c.ReloadConfig()), compare the event.Name (normalized with filepath.Clean and optionally filepath.Abs) to configFilePath and siteConfigFilePath and only sleep and call c.ReloadConfig() when it matches one of those two paths; otherwise continue. This keeps the existing event filter (event.Has(fsnotify.Write|Create|Rename)) but adds the filename check to avoid reload churn.
🤖 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 `@rest-api/api/cmd/api/main.go`:
- Around line 53-54: The flag parsing in applyConfigPathArgs accepts the next
token blindly, allowing a value like "--config-path --site-config-path=/x" to
set CONFIG_FILE_PATH to a flag token; update applyConfigPathArgs to validate
flag values for both "--config-path" and "--site-config-path": when you find a
flag key (match strings "--config-path" and "--site-config-path" in
applyConfigPathArgs), ensure the next arg exists, is not empty and does not
start with '-' (treat as another flag); if validation fails, log a fatal error
(or return an error) and exit instead of assigning CONFIG_FILE_PATH or
SITE_CONFIG_FILE_PATH, and adjust any callers to handle the error path.
In `@rest-api/api/internal/config/config.go`:
- Around line 1354-1367: The function uniqueDirs drops relative paths because
filepath.Split returns an empty dir for filenames like "config.yaml"; change the
extraction to use filepath.Dir(filepath.Clean(path)) (instead of filepath.Split)
and treat the empty/“.” result as the current directory (e.g., normalize
empty/“.” to "." or keep ".") so those directories are not skipped; update the
logic around the dir == "" check and seen[dir] to allow and record the current
directory for functions uniqueDirs and any callers that rely on it.
---
Nitpick comments:
In `@rest-api/api/cmd/api/main_test.go`:
- Around line 15-28: Add unit tests inside TestApplyConfigPathArgs (or new
subtests) to cover the parser branches for inline flag syntax and missing-value
errors: call applyConfigPathArgs with args using "--config-path=/path" and
"--site-config-path=/path" and assert the corresponding env vars
(config.ConfigFilePathEnv, config.ConfigSiteConfigFilePathEnv) are set; also
call applyConfigPathArgs with malformed/missing-value forms like "--config-path"
(end of slice) and "--site-config-path=" (empty value) and assert the env vars
remain unchanged or are cleared as the parser should behave, and add asserts for
expected behavior (error/empty) for each case to prevent regressions in
applyConfigPathArgs.
In `@rest-api/api/internal/config/config.go`:
- Around line 1338-1343: The watcher currently reloads on any
Write/Create/Rename event; restrict this to only the configured config files by
checking the event target name before calling c.ReloadConfig(). Inside the loop
where event := <-watcher.Events is handled (the block that calls event.Has and
c.ReloadConfig()), compare the event.Name (normalized with filepath.Clean and
optionally filepath.Abs) to configFilePath and siteConfigFilePath and only sleep
and call c.ReloadConfig() when it matches one of those two paths; otherwise
continue. This keeps the existing event filter
(event.Has(fsnotify.Write|Create|Rename)) but adds the filename check to avoid
reload churn.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Enterprise
Run ID: 3c228cc2-3306-4414-ad57-d95cd175eeea
📒 Files selected for processing (10)
rest-api/api/cmd/api/main.gorest-api/api/cmd/api/main_test.gorest-api/api/internal/config/config.gorest-api/api/internal/config/config_test.gorest-api/api/internal/config/testing.gorest-api/api/pkg/api/handler/settings.gorest-api/api/pkg/api/handler/settings_test.gorest-api/api/pkg/api/model/settings.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.go
Signed-off-by: Hasan Khan <hasank@nvidia.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Signed-off-by: Hasan Khan <hasank@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch open/unresolved review comments on PR `#2289`
gh pr view 2289 --repo NVIDIA/infra-controller --json reviews,reviewDecision,state,statusCheckRollup 2>/dev/null || echo "Access limited"
# List all review comments (including resolved status)
gh api repos/NVIDIA/infra-controller/pulls/2289/comments --paginate 2>/dev/null | jq '[.[] | {id, path, line, body: .body[:200], user: .user.login, resolved: (if .position == null then "outdated/resolved" else "active" end)}]' 2>/dev/null || echo "Could not fetch review comments"
# Check for review threads
gh api repos/NVIDIA/infra-controller/pulls/2289/reviews --paginate 2>/dev/null | jq '[.[] | {id, state, user: .user.login, body: .body[:200]}]' 2>/dev/null || echo "Could not fetch reviews"Length of output: 10591
🔴 Actionable (Major) — Still Open
🟡 Nitpick — Still Open
Please address the two major issues at minimum before this is production-ready. Let me know if you would like concrete diff suggestions for any of them. |
|
@coderabbitai review now changes have been done |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of applyConfigPathArgs in main.go
cat rest-api/api/cmd/api/main.go 2>/dev/null || fd -e go -p 'cmd/api/main.go' --exec cat {}
echo "=== main_test.go ==="
cat rest-api/api/cmd/api/main_test.go 2>/dev/null || fd -e go -p 'cmd/api/main_test.go' --exec cat {}
echo "=== config.go file watcher section ==="
rg -n "filepath|uniqueDirs|WatchConfig|watchedConfig|isWatched|fsnotify|Split|Dir|Clean" rest-api/api/internal/config/config.go 2>/dev/null | head -100
echo "=== uniqueDirs function ==="
ast-grep --pattern 'func uniqueDirs($$$) $$$' 2>/dev/null | head -60
echo "=== isWatchedConfigFileEvent ==="
ast-grep --pattern 'func isWatchedConfigFileEvent($$$) $$$' 2>/dev/null | head -60
echo "=== PR review comments current state ==="
gh api repos/NVIDIA/infra-controller/pulls/2289/comments --paginate 2>/dev/null | jq '[.[] | {id, path, line, body: .body[:300], user: .user.login, position, resolved: (if .position == null then "outdated/resolved" else "active" end)}]' 2>/dev/nullLength of output: 13790 🏁 Script executed: #!/bin/bash
# Resolve the two open major review comments
gh api --method PUT \
repos/NVIDIA/infra-controller/pulls/2289/comments/3370017951/replies \
2>/dev/null || true
# Resolve review comment threads via GraphQL
# First get the thread IDs for these pull request review comment IDs
gh api graphql -f query='
{
repository(owner: "NVIDIA", name: "infra-controller") {
pullRequest(number: 2289) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
body
}
}
}
}
}
}
}' 2>/dev/null | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].databaseId == 3370017951 or .comments.nodes[0].databaseId == 3370017952) | {id, isResolved, databaseId: .comments.nodes[0].databaseId}' 2>/dev/nullLength of output: 1209 🏁 Script executed: #!/bin/bash
# Get thread IDs to resolve
THREADS=$(gh api graphql -f query='
{
repository(owner: "NVIDIA", name: "infra-controller") {
pullRequest(number: 2289) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
}
}
}
}
}
}
}' 2>/dev/null)
echo "$THREADS" | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].databaseId == 3370017951 or .comments.nodes[0].databaseId == 3370017952)'
# Extract IDs and resolve them
THREAD_IDS=$(echo "$THREADS" | jq -r '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].databaseId == 3370017951 or .comments.nodes[0].databaseId == 3370017952) | .id')
for THREAD_ID in $THREAD_IDS; do
echo "Resolving thread: $THREAD_ID"
gh api graphql -f query="
mutation {
resolveReviewThread(input: {threadId: \"$THREAD_ID\"}) {
thread {
id
isResolved
}
}
}" 2>/dev/null
doneLength of output: 1648
(^_^) 🐇 |
1 participant
Description
site.phoneHomeUrlan effective runtime setting with base config, site config, and env source tracking./metadata/settingsendpoint to expose the effective phone home URL and reload status.--config-pathand--site-config-pathflags before REST API config initialization.This addresses the GitOps/file-reload path discussed in #2277 while avoiding an in-memory REST write endpoint that could diverge from the declarative config source.
Type of Change
Related Issues (Optional)
Refs #2277
Breaking Changes
Testing
Commands run:
git diff --checkgo test ./api/internal/config ./api/cmd/api ./api/pkg/api/model ./api/pkg/apigo test ./api/pkg/api/handler -run 'Test(SettingsHandler|MetadataHandler)'go test ./api/... -run '^$'go test -race ./api/internal/config ./api/cmd/api ./api/pkg/api/handler -run 'Test(SettingsHandler|MetadataHandler|ApplyConfigPathArgs|RuntimeSettings|ReloadConfig|UniqueDirs)'Additional Notes
phoneHomeUrlto avoid divergence from GitOps-managed config files.