Generate lightspeed-stack.yaml from template.yaml #90
Conversation
WalkthroughThis change deletes Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Script as scripts/generate.sh
participant FS as Filesystem
participant OC as oc process
participant YQ as yq
Dev->>Script: Run generate.sh
Script->>FS: Check for .template-params.override.env
alt Override file exists
Script->>OC: oc process template.yaml with override + template-params.dev.env
else No override file
Script->>OC: oc process template.yaml with template-params.dev.env
end
OC-->>Script: Processed template (stdout)
Script->>YQ: Extract lightspeed-stack.yaml from ConfigMap item
YQ-->>FS: Write config/lightspeed-stack.yaml
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi @keitwb. Thanks for your PR. I'm waiting for a rh-ecosystem-edge member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
I split this out to its own PR since it isn't a hard dependency of the exporter stuff and that stuff is blocked pending approvals from other teams. |
![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)
.gitignore (1)
11-11: Missing ignore rule for the YAML-based override file mentioned in the PR descriptionThe PR text states that users can create a
.template-params.override.yaml, but only the.envvariant is ignored. Either add the YAML entry or update the docs/PR text to avoid confusion and accidental commits..template-params.override.env +.template-params.override.yamlscripts/generate.sh (1)
28-30: Quote the override path to avoid word-splittingIf
$PROJECT_ROOTever contains spaces the current expansion will break.-OVERRIDE_FILE=$PROJECT_ROOT/.template-params.override.env +OVERRIDE_FILE="$PROJECT_ROOT/.template-params.override.env"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)lightspeed-stack.template.yaml(0 hunks)scripts/generate.sh(2 hunks)template-params.dev.env(1 hunks)template.yaml(4 hunks)
💤 Files with no reviewable changes (1)
- lightspeed-stack.template.yaml
🔇 Additional comments (1)
template.yaml (1)
103-112: Template parameters and dev env file out of syncThe newly added parameters are correct, but keep in mind the mismatch flagged in
template-params.dev.env; until that file is fixed, users will not be able to override these values successfully.
| if [[ -f $OVERRIDE_FILE ]]; then | ||
| OVERRIDE_PARAMS="--param-file=$OVERRIDE_FILE" | ||
| fi | ||
|
|
||
| oc process --local \ | ||
| -f $PROJECT_ROOT/template.yaml \ | ||
| ${OVERRIDE_PARAMS-} \ | ||
| --param-file=$PROJECT_ROOT/template-params.dev.env | \ | ||
| yq '.items[] | select(.kind == "ConfigMap" and .metadata.name == "lightspeed-stack-config").data."lightspeed-stack.yaml"' -r \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Override file loses precedence due to parameter-file order
oc process applies later --param-file arguments last, overriding earlier ones.
Placing ${OVERRIDE_PARAMS-} before template-params.dev.env means dev defaults will overwrite local overrides.
Swap the order (or add a dedicated -p per-param flag) so overrides truly win.
- ${OVERRIDE_PARAMS-} \
- --param-file=$PROJECT_ROOT/template-params.dev.env | \
+ --param-file=$PROJECT_ROOT/template-params.dev.env \
+ ${OVERRIDE_PARAMS-} | \🤖 Prompt for AI Agents
In scripts/generate.sh around lines 28 to 36, the override parameter file is
specified before the default dev environment parameter file, causing the
defaults to overwrite the overrides. To fix this, reorder the `oc process`
command arguments so that `${OVERRIDE_PARAMS-}` comes after
`--param-file=$PROJECT_ROOT/template-params.dev.env`, ensuring the override
parameters take precedence as intended.
There was a problem hiding this comment.
No, that is the opposite of how it actually works.
keitwb
force-pushed
the
config-from-template
branch
from
6827e7c to
6d25d25
Compare
|
/ok-to-test |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74, keitwb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
keitwb
force-pushed
the
config-from-template
branch
from
6d25d25 to
c192719
Compare
I checked to make sure it corresponds to the existing lightspeed-stack.template.yaml in terms of values. Local overrides can be added by creating a .template-params.override.yaml file in the project root.
keitwb
force-pushed
the
config-from-template
branch
from
c192719 to
0225e12
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/generate.sh (1)
32-37: Param precedence: verify which --param-file wins; also add quoting and explicit -o yamlThere’s prior discussion about which param-file takes precedence. Many CLIs apply “last wins,” but please verify oc process behavior to ensure local overrides truly win. Meanwhile, quoting improves robustness, and -o yaml makes output format explicit.
Verification (docs/web):
In `oc process`, when multiple `--param-file` flags are provided, which one takes precedence if the same key appears in both? Does the "last provided" override earlier values?If “last wins,” consider this reordering and quoting:
-oc process --local \ - -f $PROJECT_ROOT/template.yaml \ - ${OVERRIDE_PARAMS-} \ - --param-file=$PROJECT_ROOT/template-params.dev.env | \ +oc process --local -o yaml \ + -f "$PROJECT_ROOT/template.yaml" \ + --param-file="$PROJECT_ROOT/template-params.dev.env" \ + ${OVERRIDE_PARAMS+"${OVERRIDE_PARAMS[@]}"} | \ yq '.items[] | select(.kind == "ConfigMap" and .metadata.name == "lightspeed-stack-config").data."lightspeed-stack.yaml"' -r \ - > $PROJECT_ROOT/config/lightspeed-stack.yaml + > "$PROJECT_ROOT/config/lightspeed-stack.yaml"If “first wins,” keep current order but still apply quoting and -o yaml:
-oc process --local \ - -f $PROJECT_ROOT/template.yaml \ - ${OVERRIDE_PARAMS-} \ - --param-file=$PROJECT_ROOT/template-params.dev.env | \ +oc process --local -o yaml \ + -f "$PROJECT_ROOT/template.yaml" \ + ${OVERRIDE_PARAMS+"${OVERRIDE_PARAMS[@]}"} \ + --param-file="$PROJECT_ROOT/template-params.dev.env" | \ yq '.items[] | select(.kind == "ConfigMap" and .metadata.name == "lightspeed-stack-config").data."lightspeed-stack.yaml"' -r \ - > $PROJECT_ROOT/config/lightspeed-stack.yaml + > "$PROJECT_ROOT/config/lightspeed-stack.yaml"
🧹 Nitpick comments (1)
scripts/generate.sh (1)
28-31: Quote OVERRIDE_FILE and avoid word-splitting; consider args arrayUse quotes in the -f test and build OVERRIDE_PARAMS as an array to safely handle paths with spaces and avoid accidental word-splitting.
-if [[ -f $OVERRIDE_FILE ]]; then - OVERRIDE_PARAMS="--param-file=$OVERRIDE_FILE" -fi +OVERRIDE_PARAMS=() +if [[ -f "$OVERRIDE_FILE" ]]; then + OVERRIDE_PARAMS+=(--param-file "$OVERRIDE_FILE") +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)lightspeed-stack.template.yaml(0 hunks)scripts/generate.sh(2 hunks)template-params.dev.env(1 hunks)template.yaml(4 hunks)
💤 Files with no reviewable changes (1)
- lightspeed-stack.template.yaml
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- template-params.dev.env
🚧 Files skipped from review as they are similar to previous changes (1)
- template.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
🔇 Additional comments (1)
scripts/generate.sh (1)
8-9: Good: clear, single source for local overridesCentralizing the override path in OVERRIDE_FILE is clean and aligns with the PR goal.
|
/lgtm |
openshift-merge-bot
bot
merged commit d2e8433
into
rh-ecosystem-edge:main
3 participants
I checked to make sure it corresponds to the existing lightspeed-stack.template.yaml in terms of values.
Local overrides can be added by creating a
.template-params.override.yaml file in the project root.
Summary by CodeRabbit