Copilot/fix env vars order by cheyang · Pull Request #282 · sgl-project/rbg

cheyang · 2026-04-22T02:08:28Z

Ⅰ. Motivation

mergeEnvVars appended RBG-injected envs after user envs. For Kubernetes $(VAR) expansion, this breaks user vars that reference RBG vars (e.g. $(RBG_ROLE_INSTANCE_NAME)) because dependencies must appear earlier in the env list.

Ⅱ. Modifications

pkg/discovery/injector.go
- Swapped merge order in mergeEnvVars so injected RBG vars are emitted before retained user vars.
- Change:
```
// before
return append(merged, injected...)
// after
return append(injected, merged...)
```
pkg/discovery/injector_test.go
- Updated existing expected env ordering to match new contract: RBG-injected vars first, user-defined non-overlapping vars after.
- Added regression test:
  - TestDefaultInjector_InjectEnv_UserEnvReferenceRBGVarsOrder
  - Asserts DP_ADDRESS (with $(RBG_ROLE_INSTANCE_NAME)) is positioned after RBG_ROLE_INSTANCE_NAME.
Ordering contract clarified by tests
- Injected RBG envs have precedence in both override semantics and list position.
- User envs are preserved and appended when non-overlapping.

Ⅲ. Does this pull request fix one issue?

This PR addresses the env ordering issue described in this task.

Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Added unit test:
- TestDefaultInjector_InjectEnv_UserEnvReferenceRBGVarsOrder
Updated unit expectations in existing injector env merge tests to assert the new ordering behavior.

Ⅴ. Describe how to verify it

Run discovery package tests and confirm env order assertions pass:
```
go test ./pkg/discovery -count=1
```
Validate merged env order behavior in tests:
- injected RBG_* vars precede user vars
- DP_ADDRESS index is greater than RBG_ROLE_INSTANCE_NAME index

VI. Special notes for reviews

Scope is intentionally narrow: only env merge ordering + related unit tests.
No functional changes outside env injection order semantics.

Checklist

Format your code make fmt.
Add unit tests or integration tests.
Update the documentation related to the change.

Original prompt

Problem

In pkg/discovery/injector.go, the mergeEnvVars function currently places user-defined env vars before RBG built-in env vars in the final merged list. This causes a problem because Kubernetes resolves $(VAR_NAME) references in order — a variable can only reference variables defined before it.

When a user defines an env var like:

- name: DP_ADDRESS
  value: $(RBG_ROLE_INSTANCE_NAME)-0.s-rbg-lws-vglm5in-vglm5in-prefill.default

This fails because RBG_ROLE_INSTANCE_NAME is appended after DP_ADDRESS in the env list, so $(RBG_ROLE_INSTANCE_NAME) cannot be resolved.

Fix

In the mergeEnvVars function (line 188), change the return from:

return append(merged, injected...)

to:

return append(injected, merged...)

This ensures RBG built-in envs (injected) come first, and user-defined envs (merged, which are the non-overlapping existing vars) come after, allowing user envs to reference RBG variables via $(...).

Requirements

Fix the ordering in mergeEnvVars in pkg/discovery/injector.go
Update existing unit tests in pkg/discovery/injector_test.go to reflect the new ordering (RBG envs first, then user envs)
Add a new unit test that specifically verifies user env vars referencing RBG variables (e.g., $(RBG_ROLE_INSTANCE_NAME)) are placed after the RBG env vars they depend on, ensuring Kubernetes can resolve the references correctly.

gemini-code-assist · 2026-04-22T02:08:32Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copilot

Pull request overview

This PR adjusts environment variable merge ordering in the discovery injector so RBG-injected env vars appear before user-defined env vars, enabling Kubernetes $(VAR) expansion in user envs that reference injected RBG vars.

Changes:

Change mergeEnvVars to emit injected env vars before retained existing (non-overlapping) env vars.
Update injector unit tests to match the new ordering contract.
Add a regression test ensuring a user env var referencing RBG_ROLE_INSTANCE_NAME appears after that injected env var.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/discovery/injector.go	Changes env var merge ordering to put injected envs before existing envs.
pkg/discovery/injector_test.go	Updates expected env ordering and adds a regression test for Kubernetes env expansion ordering.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

This PR aims to fix Kubernetes $(VAR) expansion issues by changing how RBG-injected environment variables are merged with user-provided env vars, ensuring injected vars appear earlier in the final env list.

Changes:

Changed mergeEnvVars to emit injected env vars before retained user env vars.
Updated existing unit test expectations to match the new ordering.
Added a regression test ensuring user env vars referencing RBG vars appear after the referenced RBG env var.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/discovery/injector.go	Changes env-var merge ordering to put injected env vars before existing ones.
pkg/discovery/injector_test.go	Updates env ordering assertions and adds a test for `$(RBG_ROLE_INSTANCE_NAME)` reference ordering.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

This PR adjusts how discovery env vars are merged so Kubernetes $(VAR) expansion works when user-defined env vars reference controller-injected RBG_* variables.

Changes:

Updated mergeEnvVars to place injected env vars ahead of retained user env vars (and added logic to group existing RBG_* envs).
Updated existing injector tests to reflect the new env ordering behavior.
Added a regression test ensuring a user env var referencing $(RBG_ROLE_INSTANCE_NAME) is placed after RBG_ROLE_INSTANCE_NAME.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
pkg/discovery/injector.go	Changes env merge ordering and adds helper to identify `RBG_`-prefixed env vars.
pkg/discovery/injector_test.go	Updates expected env ordering and adds a regression test for order-dependent `$(...)` expansion.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cheyang · 2026-04-22T03:54:46Z

/gemini review

gemini-code-assist · 2026-04-22T03:54:49Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copilot

Pull request overview

Fixes Kubernetes $(VAR) env expansion issues by changing how discovery env vars are merged so RBG-injected variables are positioned before user-defined variables that may reference them.

Changes:

Updated mergeEnvVars to reorder merged env vars with RBG/system vars ahead of user vars, while preserving base-vs-derived RBG ordering.
Updated existing unit test expectations for the new env ordering contract.
Added a regression test to ensure a user env var referencing $(RBG_ROLE_INSTANCE_NAME) appears after RBG_ROLE_INSTANCE_NAME.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/discovery/injector.go	Reworks env merge ordering to support Kubernetes in-order `$(VAR)` expansion across injected and existing env vars.
pkg/discovery/injector_test.go	Updates ordering expectations and adds a regression test for user envs referencing injected RBG vars.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

This PR updates env var merging in the discovery injector so controller-injected RBG_* env vars appear before retained user env vars, preventing Kubernetes $(VAR) expansion failures when user vars reference injected vars.

Changes:

Updated mergeEnvVars ordering to prioritize injected env vars while preserving non-overlapping existing env vars.
Updated env-order expectations in existing injector tests.
Added a regression test ensuring a user env var referencing $(RBG_ROLE_INSTANCE_NAME) is ordered after RBG_ROLE_INSTANCE_NAME.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/discovery/injector.go	Changes merge logic to reorder injected vs existing env vars (including special handling for `RBG_LWP_*`).
pkg/discovery/injector_test.go	Updates expected env ordering and adds a regression test for `$(...)` expansion ordering.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

This PR fixes Kubernetes $(VAR) expansion issues by changing how RBG-injected environment variables are ordered relative to user-defined env vars during discovery env injection.

Changes:

Updated mergeEnvVars to emit existing base RBG envs first, then newly injected envs, then existing RBG_LWP_*, and finally user envs (while deduping and applying override semantics).
Adjusted existing injector env tests to match the new ordering contract and added regression coverage for user envs referencing RBG vars.
Added a new constant for the RBG_LWP_ env prefix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
pkg/discovery/injector.go	Reworks env merge ordering to support ordered `$(VAR)` expansion across multi-phase injection; adds helper prefix checks.
pkg/discovery/injector_test.go	Updates expected env ordering and adds regression/unit tests covering ordering guarantees and helper predicates.
api/workloads/constants/env.go	Introduces `EnvRBGLWPPrefix` constant used to detect `RBG_LWP_*` env vars.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

This PR fixes Kubernetes $(VAR) env expansion issues by changing mergeEnvVars to ensure RBG-injected environment variables are emitted before user-defined env vars that may reference them, while preserving multi-phase injection ordering for base vs LWP envs.

Changes:

Reworked mergeEnvVars ordering so existing base RBG_* envs come first, then newly injected envs, then existing RBG_LWP_*, then remaining user envs (with dedupe + override semantics preserved).
Updated injector tests to reflect the new env ordering contract and added regression coverage for user envs referencing injected RBG vars.
Added an API constant for the RBG_LWP_ prefix to support consistent env classification.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
pkg/discovery/injector.go	Implements deterministic env ordering to support Kubernetes `$(VAR)` expansion across multi-phase injection.
pkg/discovery/injector_test.go	Updates expectations and adds regression tests to lock in the env ordering behavior.
api/workloads/constants/env.go	Adds `EnvRBGLWPPrefix` constant for consistent classification of LWP env vars.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Agent-Logs-Url: https://github.com/cheyang/rbg/sessions/8bb0ed8e-5794-4020-ad61-1eb8e96f2a20 Co-authored-by: cheyang <[email protected]>

Signed-off-by: cheyang <[email protected]>

Syspretor

/lgtm

JasonHe-WQ · 2026-04-23T09:59:45Z

Thanks for bugs fix

Copilot AI review requested due to automatic review settings April 22, 2026 02:08

Copilot started reviewing on behalf of cheyang April 22, 2026 02:09 View session