Support single-file declarative resources via -resources startup flag#3016
Open
rajithacharith wants to merge 1 commit into
Open
Support single-file declarative resources via -resources startup flag#3016rajithacharith wants to merge 1 commit into
rajithacharith wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughServer accepts a ChangesSingle-File Declarative Resources
Sequence Diagram(s)sequenceDiagram
participant ServerStart
participant ResourceLoader
participant GetConfigsFromFile
participant splitYAMLDocuments
participant documentMatchesResourceType
participant SubstituteEnvironmentVariables
ServerStart->>ResourceLoader: start with -resources
ResourceLoader->>GetConfigsFromFile: read resources file
GetConfigsFromFile->>splitYAMLDocuments: split by ---
splitYAMLDocuments-->>GetConfigsFromFile: document slices
GetConfigsFromFile->>documentMatchesResourceType: filter by header comment
documentMatchesResourceType-->>GetConfigsFromFile: matching documents
GetConfigsFromFile->>SubstituteEnvironmentVariables: apply env substitution
SubstituteEnvironmentVariables-->>ResourceLoader: processed configs
ResourceLoader->>ResourceLoader: loadSingleResource for each config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
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 unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
backend/cmd/server/main.go (1)
119-125: ⚡ Quick winReplace hardcoded brand literals in logs.
Line 119, Line 120, and Line 125 hardcode
"ThunderID"in log messages. Is this hardcoded brand name intentional?
If not, use a named constant (or runtime config) instead of raw string literals.As per coding guidelines: “
**/*.go: Scan for hardcoded occurrences of the string literalsThunderorThunderID... suggest using a named constant or a value sourced from runtime config.”🤖 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 `@backend/cmd/server/main.go` around lines 119 - 125, The log messages in the goroutine and the two preceding logger.Info calls hardcode the brand "ThunderID"; replace those raw string literals with a single named constant or a config value (e.g., BrandName constant or cfg.Brand) and use that constant in the logger.Info calls that reference serverURL, consoleURL, and the startup message (look for logger.Info usages around serverURL, consoleURL, and the startupDuration/startupStartedAt goroutine) so all log messages use the centralized brand value instead of hardcoded strings.
🤖 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 `@backend/internal/system/declarative_resource/manager_file_test.go`:
- Around line 320-329: The test
TestGetConfigsFromFile_RelativePathResolvesFromCWD is never exercising a
relative path because it calls GetConfigsFromFile with absPath; change the test
to actually pass a relative path by switching the CWD to tmpDir (or computing a
relative path) and then call GetConfigsFromFile with "resources.yaml" (or the
relative filename) so the code path that resolves relative paths from the
current working directory is exercised; keep the test name and assertions the
same but ensure you use os.Chdir or filepath.Rel to produce a real relative path
before calling GetConfigsFromFile.
In `@backend/internal/system/declarative_resource/manager.go`:
- Around line 69-78: The code treats any trimmed `---` as a document separator,
which splits on indented `---` inside YAML block scalars; update the loop that
iterates over bytes.Split(content, []byte("\n")) to only treat a line as a
document boundary when the line is exactly `---` with no leading indentation
(i.e., check both bytes.Equal(bytes.TrimSpace(line), []byte("---")) and that the
raw line starts with `---`, e.g. bytes.HasPrefix(line, []byte("---"))); keep the
existing handling of appending `current` and resetting it when that stricter
condition is met.
- Around line 89-104: The check scans all comment lines after trimming
indentation which lets indented or later comments match; change the loop over
bytes.Split(doc, []byte("\n")) to only examine the initial top-of-file comment
block and require the '#' to be at column 0: for each line in the split, first
test if the line starts with byte('#') (i.e. line[0] == '#') and if not, break
out of the loop (stop scanning further lines); only then perform the existing
checks (normalize via strings.ToLower on the text after removing the leading '#'
and match the prefixes "resource_type:", "resource type:", "resourcetype:"
against resourceType) so indented or later comment lines are ignored.
In `@tests/integration/declarativesinglefile/single_file_mode_test.go`:
- Around line 270-271: Replace the broad negative assertion
s.NotEqual(http.StatusNoContent, resp.StatusCode, "declarative IDP should not be
deletable") with a precise equality check asserting the API contract for
immutable declarative resources: assert resp.StatusCode equals the expected
failure status (e.g., http.StatusMethodNotAllowed) by changing to
s.Equal(http.StatusMethodNotAllowed, resp.StatusCode, "declarative IDP delete
should return method not allowed"); update the assertion message accordingly and
keep the same resp variable and surrounding test flow.
- Around line 103-106: The test suite currently unconditionally overwrites and
unsets environment variables; change SingleFileModeSuite to preserve and restore
any pre-existing env values by adding a field (e.g., originalEnv
map[string]*string) and in the suite Setup (or SetupTest) record the original
value for each env key using os.LookupEnv (store nil for absent), then Setenv as
needed; in TearDown (or TearDownTest) iterate originalEnv and restore originals
(use os.Setenv for non-nil values, os.Unsetenv for nil) instead of blindly
Unsetenv; update references in methods on SingleFileModeSuite (SetupTest,
TearDownTest) accordingly so pre-existing envs are preserved.
- Around line 112-139: Add documentation covering the new -resources startup
flag and single-file declarative resources: create/update pages under
docs/content/guides/ that document the CLI usage for -resources (examples
showing absolute vs relative path behavior and startup examples), the
single-file multi-document YAML format (include the "# resource_type: <type>"
header convention and example multi-doc file), the env-var substitution syntax
and semantics ("{{.VAR}}" usage and substitution rules), and the declarative
immutability semantics (what API responses/errors to expect for mutate/delete
attempts against declaratively loaded resources); also add cross-links from
docs/content/apis.mdx to the new guide(s) so CLI/runtime config and API
immutability behavior are discoverable together.
- Around line 147-155: The teardown currently logs warnings when
testutils.RestartServer() or testutils.ObtainAdminAccessToken() fail; change
these to fail the test immediately by replacing the s.T().Logf calls with test
failures (e.g. call s.T().Fatalf or use s.Require().NoError(err) /
s.Require().NoErrorf to assert no error). Specifically update the blocks that
call testutils.RestartServer() and testutils.ObtainAdminAccessToken() so any
non-nil err triggers an immediate test failure instead of just logging.
In `@tests/integration/testutils/test_utils.go`:
- Around line 648-651: StartServer currently accepts port and zipFilePattern but
ignores them and delegates to startServerInternal which uses package-level
serverPort, causing dead parameters and potential stale global state; either
remove the unused parameters from StartServer (and clean up any callers) or
modify StartServer to pass port and zipFilePattern into startServerInternal (and
update startServerInternal signature/logic to use those values instead of the
package-level serverPort); reference StartServer, startServerInternal and the
package-level serverPort when making the change so callers and internal usage
are updated consistently.
---
Nitpick comments:
In `@backend/cmd/server/main.go`:
- Around line 119-125: The log messages in the goroutine and the two preceding
logger.Info calls hardcode the brand "ThunderID"; replace those raw string
literals with a single named constant or a config value (e.g., BrandName
constant or cfg.Brand) and use that constant in the logger.Info calls that
reference serverURL, consoleURL, and the startup message (look for logger.Info
usages around serverURL, consoleURL, and the startupDuration/startupStartedAt
goroutine) so all log messages use the centralized brand value instead of
hardcoded strings.
🪄 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 Plus
Run ID: 5b4afb23-f815-4932-b2c6-7a90a538b58b
📒 Files selected for processing (10)
backend/cmd/server/main.gobackend/internal/system/config/runtimeconfig.gobackend/internal/system/declarative_resource/loader.gobackend/internal/system/declarative_resource/manager.gobackend/internal/system/declarative_resource/manager_file_test.gobackend/internal/system/utils/configutil.gostart.ps1start.shtests/integration/declarativesinglefile/single_file_mode_test.gotests/integration/testutils/test_utils.go
Comment on lines
+112
to
+139
| func (s *SingleFileModeSuite) SetupSuite() { | ||
| // Write the fixture YAML to a temp file in the OS temp dir. | ||
| tmpDir, err := os.MkdirTemp("", "thunder-sf-test-*") | ||
| s.Require().NoError(err, "failed to create temp dir for resources fixture") | ||
|
|
||
| s.resourcesFile = filepath.Join(tmpDir, "resources.yaml") | ||
| s.Require().NoError( | ||
| os.WriteFile(s.resourcesFile, []byte(resourcesYAML), 0600), | ||
| "failed to write resources fixture", | ||
| ) | ||
|
|
||
| // Inject env vars so the server process inherits them. | ||
| for k, v := range envVars { | ||
| s.Require().NoError(os.Setenv(k, v), "failed to set env var %s", k) | ||
| } | ||
|
|
||
| // Restart the server with the -resources flag. | ||
| s.Require().NoError( | ||
| testutils.RestartServerWithResourcesFile(s.resourcesFile), | ||
| "failed to restart server with resources file", | ||
| ) | ||
|
|
||
| // Re-obtain admin token since the server process was replaced. | ||
| s.Require().NoError( | ||
| testutils.ObtainAdminAccessToken(), | ||
| "failed to obtain admin access token after server restart", | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
-resourcesstartup flag: document CLI usage, absolute/relative path behavior, and startup examples indocs/content/guides/(and cross-link fromdocs/content/apis.mdxif CLI/runtime config is indexed there).- Single-file declarative resources format: document multi-document YAML expectations,
# resource_type: <type>header usage, and env-var substitution behavior ({{.VAR}}) indocs/content/guides/. - Declarative immutability behavior in single-file mode: document expected API behavior when attempting mutation/deletion of declaratively loaded resources in
docs/content/apis.mdxand/or a relevant guide page.
As per coding guidelines, "If ANY of the above are detected and the PR does NOT include corresponding updates under docs/, post a single consolidated PR-level comment."
🤖 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 `@tests/integration/declarativesinglefile/single_file_mode_test.go` around
lines 112 - 139, Add documentation covering the new -resources startup flag and
single-file declarative resources: create/update pages under
docs/content/guides/ that document the CLI usage for -resources (examples
showing absolute vs relative path behavior and startup examples), the
single-file multi-document YAML format (include the "# resource_type: <type>"
header convention and example multi-doc file), the env-var substitution syntax
and semantics ("{{.VAR}}" usage and substitution rules), and the declarative
immutability semantics (what API responses/errors to expect for mutate/delete
attempts against declaratively loaded resources); also add cross-links from
docs/content/apis.mdx to the new guide(s) so CLI/runtime config and API
immutability behavior are discoverable together.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
rajithacharith
force-pushed
the
declarative-single-file
branch
from
9d2bd12 to
74148f8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/cmd/server/main.go (1)
119-125: ⚡ Quick winAvoid raw
ThunderIDlog labels. Is this hardcoded brand name intentional?These log messages repeat the brand name in raw string literals. If this can vary by distribution, prefer a named constant or a runtime-configured value instead of embedding
ThunderIDdirectly.As per coding guidelines: "
**/*.go: Scan for hardcoded occurrences of the string literalsThunderorThunderID... suggest using a named constant or a value sourced from runtime config`."🤖 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 `@backend/cmd/server/main.go` around lines 119 - 125, The log messages in main.go use hardcoded "ThunderID" in logger.Info calls (for serverURL, consoleURL and startup message in the goroutine) — replace those literals with a single named constant or a runtime-configured value (e.g., ServerName or appConfig.ServerName) and use that constant/value in the logger.Info calls for the messages that reference ThunderID (the calls around logger.Info("ThunderID Server URL", ...), logger.Info("ThunderID Console URL", ...), and logger.Info("ThunderID Server started", ...)); keep existing variables serverURL, consoleURL, startupDuration and startupStartedAt unchanged, only change the displayed name to the constant/config value.tests/integration/declarativesinglefile/single_file_mode_test.go (1)
52-95: ⚡ Quick winThis fixture doesn't cover the non-Go-template shielding path.
The comment says this suite includes a
{{ t(...) }}expression, but the YAML only exercises{{.SF_TEST_GITHUB_CLIENT_ID}}. Either add a literal non-Go-template expression to one document and assert it survives, or remove the claim.🤖 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 `@tests/integration/declarativesinglefile/single_file_mode_test.go` around lines 52 - 95, The test fixture constant resourcesYAML claims it exercises a non-Go-template shielding path via a "{{ t(...) }}" expression but only contains "{{.SF_TEST_GITHUB_CLIENT_ID}}"; update resourcesYAML (the const) to either include a literal non-Go-template expression (e.g. add "{{ t \"preserve\" }}" into a document field like description of sf-decl-idp-1 or sf-decl-idp-2) so the test asserts that such expressions survive, or remove/update the comment that mentions "{{ t(...) }}" to reflect the actual contents; modify the test assertions accordingly to check that the literal expression is preserved when added.
🤖 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 `@backend/internal/system/utils/configutil.go`:
- Line 199: The hardcoded placeholder format "__THUNDER_TPL_LITERAL_%d__" should
be extracted into a named constant (e.g., tplLiteralPlaceholderFmt) or a
configurable value rather than an inline string; update the code around the
creation of the placeholder (where placeholder :=
fmt.Sprintf("__THUNDER_TPL_LITERAL_%d__", idx) is used) to reference the new
constant/config, and replace any other occurrences of the same raw literal in
the package with the constant to satisfy the coding guideline against hardcoded
"Thunder" strings.
---
Nitpick comments:
In `@backend/cmd/server/main.go`:
- Around line 119-125: The log messages in main.go use hardcoded "ThunderID" in
logger.Info calls (for serverURL, consoleURL and startup message in the
goroutine) — replace those literals with a single named constant or a
runtime-configured value (e.g., ServerName or appConfig.ServerName) and use that
constant/value in the logger.Info calls for the messages that reference
ThunderID (the calls around logger.Info("ThunderID Server URL", ...),
logger.Info("ThunderID Console URL", ...), and logger.Info("ThunderID Server
started", ...)); keep existing variables serverURL, consoleURL, startupDuration
and startupStartedAt unchanged, only change the displayed name to the
constant/config value.
In `@tests/integration/declarativesinglefile/single_file_mode_test.go`:
- Around line 52-95: The test fixture constant resourcesYAML claims it exercises
a non-Go-template shielding path via a "{{ t(...) }}" expression but only
contains "{{.SF_TEST_GITHUB_CLIENT_ID}}"; update resourcesYAML (the const) to
either include a literal non-Go-template expression (e.g. add "{{ t \"preserve\"
}}" into a document field like description of sf-decl-idp-1 or sf-decl-idp-2) so
the test asserts that such expressions survive, or remove/update the comment
that mentions "{{ t(...) }}" to reflect the actual contents; modify the test
assertions accordingly to check that the literal expression is preserved when
added.
🪄 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 Plus
Run ID: 1b7e4ea3-1e58-4ec9-b8a5-d7eadd9bf49e
📒 Files selected for processing (10)
backend/cmd/server/main.gobackend/internal/system/config/runtimeconfig.gobackend/internal/system/declarative_resource/loader.gobackend/internal/system/declarative_resource/manager.gobackend/internal/system/declarative_resource/manager_file_test.gobackend/internal/system/utils/configutil.gostart.ps1start.shtests/integration/declarativesinglefile/single_file_mode_test.gotests/integration/testutils/test_utils.go
rajithacharith
force-pushed
the
declarative-single-file
branch
from
74148f8 to
5562bf4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
start.sh (1)
254-255: 💤 Low valueQuote variables to prevent word splitting (SC2086).
While
$DEBUG_PORTand$BINARY_NAMEare unlikely to contain spaces in practice, quoting them is defensive and silences the ShellCheck warnings.Suggested fix
- dlv exec --listen=:$DEBUG_PORT --headless=true --api-version=2 --accept-multiclient --continue \ - ./${BINARY_NAME} -- "${RESOURCES_ARGS[@]}" & + dlv exec --listen=:"$DEBUG_PORT" --headless=true --api-version=2 --accept-multiclient --continue \ + "./${BINARY_NAME}" -- "${RESOURCES_ARGS[@]}" &- BACKEND_PORT=$BACKEND_PORT ./${BINARY_NAME} "${RESOURCES_ARGS[@]}" & + BACKEND_PORT="$BACKEND_PORT" "./${BINARY_NAME}" "${RESOURCES_ARGS[@]}" &Also applies to: 262-262
🤖 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 `@start.sh` around lines 254 - 255, The dlv exec invocation uses unquoted variables ($DEBUG_PORT and $BINARY_NAME) which can cause word-splitting; update the dlv command (the line invoking dlv exec with --listen=:$DEBUG_PORT and ./${BINARY_NAME}) to quote those expansions (e.g., "--listen=:$DEBUG_PORT" -> "--listen=:$DEBUG_PORT" with DEBUG_PORT quoted where substituted and ./${BINARY_NAME} -> "./${BINARY_NAME}") and apply the same quoting fix to the second occurrence around line 262 so both dlv invocations safely handle values with spaces.
🤖 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 `@backend/internal/system/declarative_resource/manager.go`:
- Around line 90-95: The loop currently treats lines with only spaces/tabs as
non-empty and then checks line[0] for '#', which breaks when there's leading
whitespace before a header comment; update the checks in the scanning loop (in
manager.go) to operate on a trimmed version of the line (e.g., t :=
strings.TrimSpace(line)): use len(t) == 0 to continue for whitespace-only lines
and strings.HasPrefix(t, "#") (or check t[0] == '#') to detect header comments;
add the strings import if missing.
In `@start.ps1`:
- Around line 175-178: The catch block in start.ps1 currently logs a warning and
falls back to setting the raw value (Write-Warning +
[System.Environment]::SetEnvironmentVariable), which is inconsistent with
start.sh; change the catch in the JSON array parsing code so it fails like
start.sh: log the full error (e.g., Write-Error "Failed to parse JSON array for
key '$key': $_") and stop execution with a non-zero exit (use exit 1 or throw)
instead of calling SetEnvironmentVariable; update the catch that references
Write-Warning and SetEnvironmentVariable to use Write-Error and exit to align
behavior with start.sh.
---
Nitpick comments:
In `@start.sh`:
- Around line 254-255: The dlv exec invocation uses unquoted variables
($DEBUG_PORT and $BINARY_NAME) which can cause word-splitting; update the dlv
command (the line invoking dlv exec with --listen=:$DEBUG_PORT and
./${BINARY_NAME}) to quote those expansions (e.g., "--listen=:$DEBUG_PORT" ->
"--listen=:$DEBUG_PORT" with DEBUG_PORT quoted where substituted and
./${BINARY_NAME} -> "./${BINARY_NAME}") and apply the same quoting fix to the
second occurrence around line 262 so both dlv invocations safely handle values
with spaces.
🪄 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 Plus
Run ID: 0dda2ed6-7f6f-4387-883e-bc4e206c5491
📒 Files selected for processing (10)
backend/cmd/server/main.gobackend/internal/system/config/runtimeconfig.gobackend/internal/system/declarative_resource/loader.gobackend/internal/system/declarative_resource/manager.gobackend/internal/system/declarative_resource/manager_file_test.gobackend/internal/system/utils/configutil.gostart.ps1start.shtests/integration/declarativesinglefile/single_file_mode_test.gotests/integration/testutils/test_utils.go
rajithacharith
force-pushed
the
declarative-single-file
branch
from
5562bf4 to
7b7bb4d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/integration/testutils/test_utils.go (1)
648-651: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove the dead second parameter from
StartServer.Line 648 keeps an unused
_parameter, which preserves dead API surface and obscures intent. Remove the parameter and update callers instead of keeping a blank placeholder.Proposed cleanup
-func StartServer(port string, _ string) error { +func StartServer(port string) error { log.Println("Starting server...") return startServerInternal(port) } @@ - err := StartServer(serverPort, zipFilePattern) + err := StartServer(serverPort)As per coding guidelines: “Delete dead code cleanly… No renaming unused variables to
_prefixed names — remove them entirely unless required by an interface, callback, or framework signature.”🤖 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 `@tests/integration/testutils/test_utils.go` around lines 648 - 651, The StartServer function declares an unused second parameter; remove that dead parameter from StartServer's signature and update all callers to pass only the port string, leaving the call to startServerInternal(port) unchanged; search for the function name StartServer to adjust call sites and ensure compilation, and remove any tests or references that expect the old two-argument 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.
Inline comments:
In `@backend/internal/system/declarative_resource/manager_file_test.go`:
- Around line 33-34: originalEnvVars currently uses map[string]string which
can't distinguish unset vs set-to-empty; change its type to map[string]*string
(or maintain a companion presence map) and when recording environment in the
setup store nil for unset keys and pointer-to-value (including pointer to "")
for set keys; then update TearDownTest to restore by checking if
originalEnvVars[key] == nil then call os.Unsetenv(key) else call os.Setenv(key,
*originalEnvVars[key]); apply the same change for the other occurrences noted
(lines referenced around the variable uses and teardown logic).
In `@start.sh`:
- Around line 252-255: The dlv exec invocation and variable uses risk
word-splitting/unexpected globbing; update usages to quote variables and arrays:
ensure RESOURCES_ARGS assignment uses RESOURCES_ARGS=(-resources
"$RESOURCES_FILE") (already quoted) and call dlv with "--listen=:${DEBUG_PORT}"
and execute "./${BINARY_NAME}" quoted and pass the array as
"${RESOURCES_ARGS[@]}", i.e. quote DEBUG_PORT, BINARY_NAME and expand the array
with quotes to prevent splitting/globbing; apply the same quoting fixes to the
analogous occurrences referenced (lines 260-262).
---
Duplicate comments:
In `@tests/integration/testutils/test_utils.go`:
- Around line 648-651: The StartServer function declares an unused second
parameter; remove that dead parameter from StartServer's signature and update
all callers to pass only the port string, leaving the call to
startServerInternal(port) unchanged; search for the function name StartServer to
adjust call sites and ensure compilation, and remove any tests or references
that expect the old two-argument signature.
🪄 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 Plus
Run ID: 18505ce9-3e7e-4211-9b6b-31df4bdd48e5
📒 Files selected for processing (10)
backend/cmd/server/main.gobackend/internal/system/config/runtimeconfig.gobackend/internal/system/declarative_resource/loader.gobackend/internal/system/declarative_resource/manager.gobackend/internal/system/declarative_resource/manager_file_test.gobackend/internal/system/utils/configutil.gostart.ps1start.shtests/integration/declarativesinglefile/single_file_mode_test.gotests/integration/testutils/test_utils.go
Comment on lines
+33
to
+34
| originalEnvVars map[string]string | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve originally-empty environment variables in teardown.
originalEnvVars cannot distinguish “unset” from “set to empty”, so TearDownTest may unset vars that originally existed with "", causing cross-test state drift.
Proposed fix
type ResourcesFileTestSuite struct {
suite.Suite
- originalEnvVars map[string]string
+ originalEnvVars map[string]struct {
+ value string
+ exists bool
+ }
}
func (s *ResourcesFileTestSuite) SetupTest() {
- s.originalEnvVars = make(map[string]string)
+ s.originalEnvVars = make(map[string]struct {
+ value string
+ exists bool
+ })
}
func (s *ResourcesFileTestSuite) TearDownTest() {
- for key, value := range s.originalEnvVars {
- if value == "" {
+ for key, snapshot := range s.originalEnvVars {
+ if !snapshot.exists {
_ = os.Unsetenv(key)
} else {
- _ = os.Setenv(key, value)
+ _ = os.Setenv(key, snapshot.value)
}
}
}
func (s *ResourcesFileTestSuite) setEnvVar(key, value string) {
if _, exists := s.originalEnvVars[key]; !exists {
if orig, has := os.LookupEnv(key); has {
- s.originalEnvVars[key] = orig
+ s.originalEnvVars[key] = struct {
+ value string
+ exists bool
+ }{value: orig, exists: true}
} else {
- s.originalEnvVars[key] = ""
+ s.originalEnvVars[key] = struct {
+ value string
+ exists bool
+ }{exists: false}
}
}
s.Require().NoError(os.Setenv(key, value))
}Also applies to: 45-50, 55-60
🤖 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 `@backend/internal/system/declarative_resource/manager_file_test.go` around
lines 33 - 34, originalEnvVars currently uses map[string]string which can't
distinguish unset vs set-to-empty; change its type to map[string]*string (or
maintain a companion presence map) and when recording environment in the setup
store nil for unset keys and pointer-to-value (including pointer to "") for set
keys; then update TearDownTest to restore by checking if originalEnvVars[key] ==
nil then call os.Unsetenv(key) else call os.Setenv(key, *originalEnvVars[key]);
apply the same change for the other occurrences noted (lines referenced around
the variable uses and teardown logic).
7b7bb4d to
5bd49ec
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/internal/system/declarative_resource/manager_file_test.go (1)
33-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve “set but empty” env vars during teardown.
Line 46 currently treats an original empty value as “unset”, so tests can mutate global env state across cases. Track existence separately from value when snapshotting env vars.
Proposed fix
type ResourcesFileTestSuite struct { suite.Suite - originalEnvVars map[string]string + originalEnvVars map[string]struct { + value string + exists bool + } } func (s *ResourcesFileTestSuite) SetupTest() { - s.originalEnvVars = make(map[string]string) + s.originalEnvVars = make(map[string]struct { + value string + exists bool + }) } func (s *ResourcesFileTestSuite) TearDownTest() { - for key, value := range s.originalEnvVars { - if value == "" { + for key, snapshot := range s.originalEnvVars { + if !snapshot.exists { _ = os.Unsetenv(key) } else { - _ = os.Setenv(key, value) + _ = os.Setenv(key, snapshot.value) } } } func (s *ResourcesFileTestSuite) setEnvVar(key, value string) { if _, exists := s.originalEnvVars[key]; !exists { if orig, has := os.LookupEnv(key); has { - s.originalEnvVars[key] = orig + s.originalEnvVars[key] = struct { + value string + exists bool + }{value: orig, exists: true} } else { - s.originalEnvVars[key] = "" + s.originalEnvVars[key] = struct { + value string + exists bool + }{exists: false} } } s.Require().NoError(os.Setenv(key, value)) }Also applies to: 45-50, 55-60
🧹 Nitpick comments (1)
tests/integration/declarativesinglefile/single_file_mode_test.go (1)
52-57: ⚡ Quick winAlign the fixture comment with the actual YAML.
These comments say the fixture includes a
{{ t(...) }}expression to verify non-Go-template pass-through, but the YAML below no longer contains one. Either add that expression to the fixture or remove the claim so the suite does not overstate its coverage.🤖 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 `@tests/integration/declarativesinglefile/single_file_mode_test.go` around lines 52 - 57, The comment above the resourcesYAML fixture is stale: it claims the YAML contains a {{ t(...) }} expression but the fixture no longer does; update the comment or the fixture. Either add a non-Go-template token like "{{ t(\"something\") }}" into the resourcesYAML test fixture so the description matches (ensuring the expression appears in an IDP description string), or remove the sentence that references "{{ t(...) }}" from the comment so the comment accurately reflects the current resourcesYAML contents; locate the resourcesYAML variable and the surrounding comment block to make the change.
🤖 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 `@start.ps1`:
- Around line 317-327: The debug branch starts Delve via Start-Process using
$dlvArgs but never sets $env:BACKEND_PORT, so the debugged server won't inherit
the intended port; before calling Start-Process (where $dlvArgs, $serverExecPath
and $resourcesArgs are used) export the backend port into the environment (e.g.
set $env:BACKEND_PORT = $BACKEND_PORT) so the dlv-launched process inherits it;
alternatively, if using PowerShell Core with Start-Process -Environment, pass a
dictionary containing BACKEND_PORT to Start-Process, but the simplest fix is to
set $env:BACKEND_PORT just prior to the Start-Process call.
In `@start.sh`:
- Around line 252-255: The debug launch using dlv exec does not pass
BACKEND_PORT into the server process, so the binary falls back to its default
port; update the dlv exec invocation that uses RESOURCES_ARGS, DEBUG_PORT and
BINARY_NAME to export/pass BACKEND_PORT into the debugged process (mirror the
non-debug branch behavior) so the server receives BACKEND_PORT=$BACKEND_PORT
when started under dlv; locate the dlv exec line and modify the invocation to
include BACKEND_PORT in the environment for the debugged binary.
---
Nitpick comments:
In `@tests/integration/declarativesinglefile/single_file_mode_test.go`:
- Around line 52-57: The comment above the resourcesYAML fixture is stale: it
claims the YAML contains a {{ t(...) }} expression but the fixture no longer
does; update the comment or the fixture. Either add a non-Go-template token like
"{{ t(\"something\") }}" into the resourcesYAML test fixture so the description
matches (ensuring the expression appears in an IDP description string), or
remove the sentence that references "{{ t(...) }}" from the comment so the
comment accurately reflects the current resourcesYAML contents; locate the
resourcesYAML variable and the surrounding comment block to make the change.
🪄 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 Plus
Run ID: 304a7fdc-b044-4842-a121-8a93892895d4
📒 Files selected for processing (10)
backend/cmd/server/main.gobackend/internal/system/config/runtimeconfig.gobackend/internal/system/declarative_resource/loader.gobackend/internal/system/declarative_resource/manager.gobackend/internal/system/declarative_resource/manager_file_test.gobackend/internal/system/utils/configutil.gostart.ps1start.shtests/integration/declarativesinglefile/single_file_mode_test.gotests/integration/testutils/test_utils.go
rajithacharith
force-pushed
the
declarative-single-file
branch
4 times, most recently
from
2a4dd2e to
92c2547
Compare
rajithacharith
force-pushed
the
declarative-single-file
branch
from
92c2547 to
3234072
Compare
rajithacharith
force-pushed
the
declarative-single-file
branch
from
3234072 to
ef01904
Compare
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.
Purpose
Removes the bash-level YAML document-splitting logic from
start.sh/start.ps1and moves the responsibility into the server binary. Thunder now accepts a-resources <file>startup flag and reads all declarative resources from a single multi-document YAML file, eliminating the need for separate per-type directories.Approach
Binary flag (
-resources) —main.goregisters a-resourcesflag, resolves it to an absolute path, and stores it viaconfig.SetResourcesFile().ServerRuntimegains aResourcesFilefield (added via a separate setter to keepInitializeServerRuntime's signature unchanged).Per-type loading —
ResourceLoader.LoadResources()checksResourcesFile; when set, it calls the newGetConfigsFromFile(filePath, resourceType)instead ofGetConfigs(dir). Resource type is derived from the loader's directory name withstrings.TrimSuffix(dir, "s"), which covers all 14 built-in types.Multi-document YAML parsing —
splitYAMLDocumentssplits on---separators.documentMatchesResourceTypematches a# resource_type: <type>comment header (supportsresource_type:,resource type:, andresourcetype:prefixes, quoted values). Env-var substitution ({{.Var}}) is applied per-document so that non-Go-template expressions in other documents (e.g.,{{ t(signin:title) }}in flow pages) do not trigger parse errors.Non-Go-template expression shielding —
SubstituteEnvironmentVariablesinconfigutil.gonow callsshieldNonGoTemplateExpressionsbefore parsing, replacing unknown{{ ... }}patterns (UI expressions, bare identifiers like{{appName}}) with unique placeholders that are restored after template execution. This fixes the pre-existingfunction "t" not definederror when flow YAML and env-var-using YAML coexisted in the same file.Start scripts —
start.shandstart.ps1are simplified: the old document-splitting logic (~100 lines each) is replaced by a lean env-file loader, and-resources "$RESOURCES_FILE"is forwarded to the binary.Tests:
manager_file_test.go— unit tests forsplitYAMLDocuments,documentMatchesResourceType, andGetConfigsFromFile(21 cases covering env-var substitution, non-Go-template pass-through, error paths, multi-type files)tests/integration/declarativesinglefile/— integration test suite that restarts the server with-resources, verifies resources are loaded (OU, two IDPs including env-var substitution), checks collection listing, and confirms declarative resources are immutableRelated Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Bug Fixes
Tests