MGMT-24006: Add netris integration to the installation flow - #72
Conversation
|
@danmanor: This pull request references MGMT-24006 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughAdds AAP runtime configuration support: ignores gitignored secrets, documents selectable network backends (ESI or Netris), adds network-backend and AAP-configuration docs, introduces overlay ConfigMap generators and example env/secret files for caas-ci and development, adds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
danmanor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
overlays/caas-ci/kustomization.yaml (1)
58-72: Kustomize literals intentionally ESI, but the companionfiles/configuration.envflips to Netris at runtime.
NETWORK_CLASS=esihere will be overwritten tonetrisbyconfigure-fulfillment-ig.shbecause the overlay'sconfiguration.envsets it. That appears to be the design, but it meanskustomize build overlays/caas-ciand the actual running state diverge — worth documenting (or setting the literal tonetristo match the file shipped in the same overlay) to avoid confusion during troubleshooting.Also: the PR description mentions a
components/netrisKustomize Component that overrides these literals viabehavior: merge. That component is not among the overlays here, so overlays currently rely solely on the runtime script to switch backends — consider clarifying the intended relationship between the component-based and script-based paths in docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overlays/caas-ci/kustomization.yaml` around lines 58 - 72, The configMapGenerator entry for name cluster-fulfillment-ig sets NETWORK_CLASS=esi which is later overwritten at runtime by the overlay's files/configuration.env and configure-fulfillment-ig.sh (so kustomize build and runtime differ); to fix, either change the literal NETWORK_CLASS in the configMapGenerator to match the intended runtime value (netris) or add documentation in the overlay explaining that configure-fulfillment-ig.sh and files/configuration.env will flip NETWORK_CLASS, and if the components/netris Kustomize component (behavior: merge) is intended to control this, ensure that component is included in the overlay or document the intended component-vs-script precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/network-backend.md`:
- Around line 10-19: The doc currently implies both env files must exist; update
the text in docs/network-backend.md to mark secrets.env as optional by stating
that secrets.env is gitignored and may be absent in overlays, and note that
scripts/configure-fulfillment-ig.sh will skip missing secrets.env when patching
the cluster; keep configuration.env described as tracked and required while
clarifying that secrets.env contains sensitive credentials and is optional
per-overlay.
- Around line 72-80: Update the docs to explicitly state that values in
`secrets.env` must be plain-text (not base64-encoded) because the deployment
script base64-encodes these values when patching the Kubernetes Secret `data`;
if users pre-encode them they will end up double-encoded and unusable in the job
pods. Mention `secrets.env` by name, note the script behavior of base64-encoding
prior to writing to the Secret `data`, and add a short warning line advising
users to provide plaintext credentials (e.g., NETRIS_PASSWORD, SERVER_SSH_KEY,
AWS_*).
- Around line 32-40: Remove the leading shell prompt markers from the
command-only code blocks so they pass MD014; update the examples that reference
./scripts/setup.sh and ./scripts/configure-fulfillment-ig.sh to show raw
commands (e.g., replace "$ ./scripts/setup.sh" with "./scripts/setup.sh" and "$
INSTALLER_NAMESPACE=<project-name> ./scripts/configure-fulfillment-ig.sh" with
"INSTALLER_NAMESPACE=<project-name> ./scripts/configure-fulfillment-ig.sh")
while keeping the surrounding fenced code blocks intact.
In `@overlays/caas-ci/files/configuration.env`:
- Around line 1-44: The overlay contains hardcoded Netris identifiers
(NETRIS_CONTROLLER_URL, NETRIS_USERNAME, NETRIS_SITE_ID, NETRIS_TENANT_ID,
NETRIS_MGMT_VPC_ID, NETRIS_RESOURCE_CLASS_MAP, etc.) copied from dev — verify
and replace these with CI-appropriate identifiers or document they are
placeholders for CaaS CI; ensure NETRIS_RESOURCE_CLASS_MAP remains a
JSON-encoded string (since scripts/configure-fulfillment-ig.sh patches it into
the ConfigMap as a quoted string) and confirm any consumers (AAP templates or
external steps) parse it as JSON rather than expecting a pre-parsed object;
finally add a short header comment above the Netris block marking which keys are
CI-infrastructure IDs vs. editable environment settings so operators know which
values must be customized.
In `@overlays/development/files/configuration.env`:
- Around line 1-44: Replace the committed Netris defaults in
development/files/configuration.env with safe ESI defaults (e.g. set
NETWORK_CLASS=esi, NETWORK_STEPS_COLLECTION=esi.steps and clear
network/controller/SSH values like NETRIS_CONTROLLER_URL, NETRIS_USERNAME,
SERVER_SSH_BASTION_HOST, NETRIS_SITE_ID, NETRIS_TENANT_ID, NETRIS_MGMT_VPC_ID,
NETRIS_RESOURCE_CLASS_MAP), move the current Netris values into a new
development/files/configuration.env.example as an opt-in example, add
configuration.env to .gitignore, and update scripts/configure-fulfillment-ig.sh
(and scripts/setup.sh call site if needed) to no-op or skip configuration when
configuration.env is absent to avoid silently applying environment-specific
Netris settings.
In `@README.md`:
- Around line 283-299: The README claim that configuration.env "ships with ESI
defaults" is inconsistent with committed overlays that set NETWORK_CLASS=netris
and include live Netris values; update either the committed files or the docs:
either change overlays/development/files/configuration.env and
overlays/caas-ci/files/configuration.env to use ESI/default placeholder values
(e.g., remove live Netris controller entries and set NETWORK_CLASS to the
documented default) or revise README.md wording to state that the repository
ships overlays configured for Netris (mention NETWORK_CLASS=netris) and that
secrets must be supplied; also add or link the full variable list (from
docs/network-backend.md or the PR ConfigMap/Secret list) into this section so
the variable reference is immediately available.
In `@scripts/configure-fulfillment-ig.sh`:
- Around line 20-33: The load_env_file function currently splits on lines and
loses multiline secret values (e.g., SSH private keys) and elsewhere the script
uses echo -n and GNU-only base64 -w0 which corrupts values starting with '-' and
wrapped base64 output; update load_env_file to read the file line-by-line with
IFS= read -r line, skip blank/comments, split each line at the first '=' with
key="${line%%=*}" and value="${line#*=}" so value preserves embedded
newlines/characters, trim comments from key only, and export using export
"${key}=${value}"; change any use of echo -n to printf '%s' when emitting secret
content (references: SERVER_SSH_KEY, SERVER_SSH_BASTION_KEY), and replace base64
-w0 with a portable pipeline like base64 | tr -d '\n' (or use base64 -w0 when
available) to ensure no line-wrapping and avoid corrupting secrets.
In `@scripts/setup.sh`:
- Around line 215-216: The variables INSTALLER_NAMESPACE and
INSTALLER_KUSTOMIZE_OVERLAY are defined but not exported, so they are not
visible to the child script configure-fulfillment-ig.sh; update the parent
script to export INSTALLER_NAMESPACE and INSTALLER_KUSTOMIZE_OVERLAY (or export
them where first defined) before invoking ./scripts/configure-fulfillment-ig.sh
so the child receives the correct namespace and kustomize overlay instead of
falling back to its hardcoded defaults.
---
Nitpick comments:
In `@overlays/caas-ci/kustomization.yaml`:
- Around line 58-72: The configMapGenerator entry for name
cluster-fulfillment-ig sets NETWORK_CLASS=esi which is later overwritten at
runtime by the overlay's files/configuration.env and configure-fulfillment-ig.sh
(so kustomize build and runtime differ); to fix, either change the literal
NETWORK_CLASS in the configMapGenerator to match the intended runtime value
(netris) or add documentation in the overlay explaining that
configure-fulfillment-ig.sh and files/configuration.env will flip NETWORK_CLASS,
and if the components/netris Kustomize component (behavior: merge) is intended
to control this, ensure that component is included in the overlay or document
the intended component-vs-script precedence.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ea98133-46bc-4dac-a17a-ab15d4274024
📒 Files selected for processing (10)
.gitignoreREADME.mddocs/network-backend.mdoverlays/caas-ci/files/configuration.envoverlays/caas-ci/kustomization.yamloverlays/development/files/configuration.envoverlays/development/kustomization.yamloverlays/vmaas-ci/kustomization.yamlscripts/configure-fulfillment-ig.shscripts/setup.sh
| Each overlay contains two env files under `files/`: | ||
|
|
||
| | File | Tracked in git | Purpose | | ||
| |------|---------------|---------| | ||
| | `configuration.env` | Yes | Non-sensitive settings (network class, domains, Netris connection) | | ||
| | `secrets.env` | No (gitignored) | Sensitive credentials (passwords, SSH keys, AWS keys) | | ||
|
|
||
| The `scripts/configure-fulfillment-ig.sh` script reads both files and patches the | ||
| `cluster-fulfillment-ig` ConfigMap and Secret on the cluster. The setup script | ||
| (`setup.sh`) calls this automatically. |
There was a problem hiding this comment.
Mark secrets.env as optional.
Line 10 says every overlay contains two env files, but scripts/configure-fulfillment-ig.sh skips missing files, and secrets.env is gitignored. This should not imply credentials files must exist in every overlay.
📝 Proposed wording
-Each overlay contains two env files under `files/`:
+Each overlay contains a tracked configuration file under `files/`; add a local
+`secrets.env` only when credential overrides are needed:
| File | Tracked in git | Purpose |
|------|---------------|---------|
| `configuration.env` | Yes | Non-sensitive settings (network class, domains, Netris connection) |
-| `secrets.env` | No (gitignored) | Sensitive credentials (passwords, SSH keys, AWS keys) |
+| `secrets.env` | No (gitignored, optional) | Sensitive credentials (passwords, SSH keys, AWS keys) |
-The `scripts/configure-fulfillment-ig.sh` script reads both files and patches the
-`cluster-fulfillment-ig` ConfigMap and Secret on the cluster. The setup script
-(`setup.sh`) calls this automatically.
+The `scripts/configure-fulfillment-ig.sh` script reads `configuration.env` and,
+when present, `secrets.env`, then patches the `cluster-fulfillment-ig` ConfigMap
+and Secret on the cluster. The setup script (`setup.sh`) calls this automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/network-backend.md` around lines 10 - 19, The doc currently implies both
env files must exist; update the text in docs/network-backend.md to mark
secrets.env as optional by stating that secrets.env is gitignored and may be
absent in overlays, and note that scripts/configure-fulfillment-ig.sh will skip
missing secrets.env when patching the cluster; keep configuration.env described
as tracked and required while clarifying that secrets.env contains sensitive
credentials and is optional per-overlay.
| ```bash | ||
| $ ./scripts/setup.sh | ||
| ``` | ||
|
|
||
| For manual deployments, run the configuration script standalone after applying | ||
| kustomize: | ||
|
|
||
| ```bash | ||
| $ INSTALLER_NAMESPACE=<project-name> ./scripts/configure-fulfillment-ig.sh |
There was a problem hiding this comment.
Remove prompt markers from command-only examples.
Lines 33 and 40 trigger markdownlint MD014 because the examples show $ prompts without command output.
🧹 Proposed lint fix
```bash
- $ ./scripts/setup.sh
+ ./scripts/setup.sh
```
@@
```bash
-$ INSTALLER_NAMESPACE=<project-name> ./scripts/configure-fulfillment-ig.sh
+INSTALLER_NAMESPACE=<project-name> ./scripts/configure-fulfillment-ig.sh
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 33-33: Dollar signs used before commands without showing output
(MD014, commands-show-output)
[warning] 40-40: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/network-backend.md` around lines 32 - 40, Remove the leading shell
prompt markers from the command-only code blocks so they pass MD014; update the
examples that reference ./scripts/setup.sh and
./scripts/configure-fulfillment-ig.sh to show raw commands (e.g., replace "$
./scripts/setup.sh" with "./scripts/setup.sh" and "$
INSTALLER_NAMESPACE=<project-name> ./scripts/configure-fulfillment-ig.sh" with
"INSTALLER_NAMESPACE=<project-name> ./scripts/configure-fulfillment-ig.sh")
while keeping the surrounding fenced code blocks intact.
aeb4051 to
ae04ec5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/aap-configuration.sh (1)
20-34:⚠️ Potential issue | 🟠 MajorMultiline SSH key values are still silently truncated.
The
echo -n/base64 -w0portability concerns from the prior review have been addressed (lines 86–86 now useprintf '%s' ... | base64 | tr -d '\n'), but the line-oriented parser here still cannot represent multiline values. A plaintextSERVER_SSH_KEY/SERVER_SSH_BASTION_KEYspanning PEM lines inosac-aap-secrets.envwill only capture the first line (-----BEGIN OPENSSH PRIVATE KEY-----), and the subsequent PEM body lines will be parsed as bogusKEY=VALUEpairs (or skipped), producing a corrupt/invalid Secret. Additionally,printf '%s'on line 86 omits the trailing newline that OpenSSH typically requires after the END marker.Given the docs explicitly require plaintext secrets including SSH keys, this needs either a documented single-line encoding (e.g.
\n-escaped, decoded here) or a real multiline-aware parser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 300-308: The README currently says "By default the network backend
is ESI" which conflicts with earlier lines stating the development and caas-ci
overlays ship with Netris defaults; update the Network Backend Configuration
(CaaS) paragraph to clarify that the kustomize base defaults to ESI while the
development and caas-ci overlay env files override this to Netris, and show how
to switch by setting NETWORK_CLASS=esi or NETWORK_CLASS=netris in
osac-aap-configuration.env and by filling the Netris-specific variables
(referencing NETWORK_CLASS and NETRIS_RESOURCE_CLASS_MAP) so a new user
understands base vs overlay defaults.
In `@scripts/aap-configuration.sh`:
- Around line 11-12: The namespace extraction using
INSTALLER_NAMESPACE=${INSTALLER_NAMESPACE:-$(grep "^namespace:"
"overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml" | awk '{print
$2}')} is fragile to CRLF, inline comments and quotes; update the assignment to
either use yq (e.g., yq eval '.namespace'
overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml) when yq is
available, or if not available sanitize the file content first by stripping CRs,
removing inline “#” comments and surrounding quotes and trimming whitespace
before extracting the value (referencing INSTALLER_NAMESPACE and
overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml to locate the code
path), and ensure the script still exits with error if the resulting
INSTALLER_NAMESPACE is empty.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f5b4ab6-3524-4ffa-8114-f4f13091b659
📒 Files selected for processing (15)
.gitignoreREADME.mdbase/osac-aapbase/osac-fulfillment-servicebase/osac-operatordocs/aap-configuration.mddocs/network-backend.mdoverlays/caas-ci/files/osac-aap-configuration.envoverlays/caas-ci/files/osac-aap-secrets.env.exampleoverlays/caas-ci/kustomization.yamloverlays/development/files/osac-aap-configuration.envoverlays/development/files/osac-aap-secrets.env.exampleoverlays/development/kustomization.yamlscripts/aap-configuration.shscripts/setup.sh
💤 Files with no reviewable changes (3)
- base/osac-fulfillment-service
- base/osac-operator
- base/osac-aap
✅ Files skipped from review due to trivial changes (8)
- .gitignore
- overlays/development/files/osac-aap-secrets.env.example
- overlays/development/kustomization.yaml
- overlays/caas-ci/files/osac-aap-secrets.env.example
- docs/network-backend.md
- overlays/caas-ci/files/osac-aap-configuration.env
- overlays/development/files/osac-aap-configuration.env
- docs/aap-configuration.md
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/setup.sh
- overlays/caas-ci/kustomization.yaml
| INSTALLER_NAMESPACE=${INSTALLER_NAMESPACE:-$(grep "^namespace:" "overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml" | awk '{print $2}')} | ||
| [[ -z "${INSTALLER_NAMESPACE}" ]] && echo "ERROR: Could not determine namespace from overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml" && exit 1 |
There was a problem hiding this comment.
Namespace extraction is fragile to formatting of kustomization.yaml.
grep "^namespace:" | awk '{print $2}' will break or return the wrong value if the field has a trailing \r (CRLF-edited file), an inline # comment, or is quoted (namespace: "osac-devel" keeps the quotes). Consider using yq if available, or at minimum stripping CR and trailing comments before assigning. Low likelihood in current overlays, but the error path (exit 1) only triggers on empty, not malformed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/aap-configuration.sh` around lines 11 - 12, The namespace extraction
using INSTALLER_NAMESPACE=${INSTALLER_NAMESPACE:-$(grep "^namespace:"
"overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml" | awk '{print
$2}')} is fragile to CRLF, inline comments and quotes; update the assignment to
either use yq (e.g., yq eval '.namespace'
overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml) when yq is
available, or if not available sanitize the file content first by stripping CRs,
removing inline “#” comments and surrounding quotes and trimming whitespace
before extracting the value (referencing INSTALLER_NAMESPACE and
overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml to locate the code
path), and ensure the script still exits with error if the resulting
INSTALLER_NAMESPACE is empty.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/aap-configuration.sh (1)
61-78: Optional: clean uppatch_filevia trap to avoid leaking undererrexit.If
oc patch(line 75) fails,set -o errexitaborts before line 78 and the temp file is leaked in/tmp. Low impact, but a one-liner makes it robust:♻️ Proposed refactor
# --- ConfigMap overrides --- patch_file=$(mktemp) +trap 'rm -f "${patch_file}"' EXIT has_cm_overrides=false @@ if [[ "${has_cm_overrides}" == "true" ]]; then echo "Applying cluster-fulfillment-ig configmap overrides..." oc patch configmap/cluster-fulfillment-ig -n "${INSTALLER_NAMESPACE}" \ --patch-file="${patch_file}" --type=merge fi -rm -f "${patch_file}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-configuration.sh` around lines 61 - 78, Add a safe cleanup trap so the temporary file created in patch_file is always removed even if errexit aborts (e.g., before the oc patch completes). After creating patch_file with mktemp, register a trap that runs rm -f "${patch_file}" on EXIT (or ERR/EXIT) and ensure the trap references the same patch_file variable; keep existing explicit rm -f "${patch_file}" at the end or rely on the EXIT trap but do not remove the trap registration prematurely. This change affects the patch_file lifecycle around the mktemp/oc patch/has_cm_overrides block and prevents leaking the temp file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/aap-configuration.sh`:
- Around line 61-78: Add a safe cleanup trap so the temporary file created in
patch_file is always removed even if errexit aborts (e.g., before the oc patch
completes). After creating patch_file with mktemp, register a trap that runs rm
-f "${patch_file}" on EXIT (or ERR/EXIT) and ensure the trap references the same
patch_file variable; keep existing explicit rm -f "${patch_file}" at the end or
rely on the EXIT trap but do not remove the trap registration prematurely. This
change affects the patch_file lifecycle around the mktemp/oc
patch/has_cm_overrides block and prevents leaking the temp file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdc4351b-5431-4d5e-9a94-d3d973123918
📒 Files selected for processing (12)
.gitignoreREADME.mddocs/aap-configuration.mddocs/network-backend.mdoverlays/caas-ci/files/osac-aap-configuration.envoverlays/caas-ci/files/osac-aap-secrets.env.exampleoverlays/caas-ci/kustomization.yamloverlays/development/files/osac-aap-configuration.envoverlays/development/files/osac-aap-secrets.env.exampleoverlays/development/kustomization.yamlscripts/aap-configuration.shscripts/setup.sh
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- overlays/development/files/osac-aap-secrets.env.example
- overlays/caas-ci/files/osac-aap-secrets.env.example
- overlays/development/files/osac-aap-configuration.env
- docs/network-backend.md
- docs/aap-configuration.md
🚧 Files skipped from review as they are similar to previous changes (3)
- overlays/development/kustomization.yaml
- overlays/caas-ci/files/osac-aap-configuration.env
- README.md
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/aap-configuration.sh (2)
61-78: Add atrapto guarantee the temp patch file is cleaned up.With
set -e, ifoc patchfails on line 75, line 78'srm -fnever runs and the temp file leaks (and may contain rendered config values). A one-linetrapmakes cleanup unconditional.♻️ Proposed change
# --- ConfigMap overrides --- patch_file=$(mktemp) +trap 'rm -f "${patch_file}"' EXIT has_cm_overrides=false @@ if [[ "${has_cm_overrides}" == "true" ]]; then echo "Applying cluster-fulfillment-ig configmap overrides..." oc patch configmap/cluster-fulfillment-ig -n "${INSTALLER_NAMESPACE}" \ --patch-file="${patch_file}" --type=merge fi -rm -f "${patch_file}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-configuration.sh` around lines 61 - 78, The temporary patch file created as patch_file can leak if oc patch fails because set -e causes early exit before the final rm -f; add a one-line trap to always remove the temp file (e.g., trap 'rm -f "${patch_file}"' EXIT) immediately after creating patch_file so cleanup runs on exit/failure; ensure the trap references the same variable name patch_file and leave the existing rm -f as a harmless final cleanup.
18-19: Stale comment:set -ais no longer used.The comment refers to
set -asemantics, but the implementation switched to a hand-rolledload_env_file()function that uses an explicit[[ -z "${!key:-}" ]]precedence check. Update the comment to reflect what actually happens, otherwise future readers will look for aset -ablock that doesn't exist.📝 Proposed comment update
-# Source env files. Shell environment variables take precedence (set -a exports -# only new variables; existing ones are not overwritten by source). +# Load env files. Pre-existing shell environment variables take precedence: +# load_env_file() only exports a key if it is currently unset/empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/aap-configuration.sh` around lines 18 - 19, The existing comment incorrectly references set -a; update it to describe the current hand-rolled loader: explain that load_env_file() is used to parse env files and that shell environment variables take precedence due to the explicit precedence check ([[ -z "${!key:-}" ]]) before exporting each key, so only unset variables from the file are exported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/aap-configuration.sh`:
- Around line 64-71: The current loop in the script writes CM_VARS values as
single-quoted YAML scalars which break on embedded newlines; update the loop
that writes to "${patch_file}" (the block referencing CM_VARS, escaped, and
has_cm_overrides) to detect multi-line values and either emit a YAML block
scalar (using a "|" followed by properly indented lines) or JSON-encode the
entire value (e.g., via jq -Rs .) and write the encoded value as the field
value; ensure you remove the current single-quote escaping (escaped) for
multi-line cases and set has_cm_overrides=true as before so oc patch
--patch-file receives valid YAML/JSON for multiline CM_VARS.
- Around line 27-29: The code currently strips inline comments only from the key
(variable key) but leaves trailing inline comments and leading spaces in the
value; update the value processing in scripts/aap-configuration.sh where key and
value are parsed so that after extracting value you first remove any inline “#”
comment (e.g., value = value before the first #) and then trim leading/trailing
whitespace from value before using it; reference the existing variables key and
value in your change (mirror the key handling: apply value="${value%%#*}" then
apply a trim routine) so ConfigMap entries don’t include the inline comment or
stray spaces.
---
Nitpick comments:
In `@scripts/aap-configuration.sh`:
- Around line 61-78: The temporary patch file created as patch_file can leak if
oc patch fails because set -e causes early exit before the final rm -f; add a
one-line trap to always remove the temp file (e.g., trap 'rm -f "${patch_file}"'
EXIT) immediately after creating patch_file so cleanup runs on exit/failure;
ensure the trap references the same variable name patch_file and leave the
existing rm -f as a harmless final cleanup.
- Around line 18-19: The existing comment incorrectly references set -a; update
it to describe the current hand-rolled loader: explain that load_env_file() is
used to parse env files and that shell environment variables take precedence due
to the explicit precedence check ([[ -z "${!key:-}" ]]) before exporting each
key, so only unset variables from the file are exported.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e50c9191-5af9-491f-94c0-a4d04bd09811
📒 Files selected for processing (12)
.gitignoreREADME.mddocs/aap-configuration.mddocs/network-backend.mdoverlays/caas-ci/files/osac-aap-configuration.envoverlays/caas-ci/files/osac-aap-secrets.env.exampleoverlays/caas-ci/kustomization.yamloverlays/development/files/osac-aap-configuration.envoverlays/development/files/osac-aap-secrets.env.exampleoverlays/development/kustomization.yamlscripts/aap-configuration.shscripts/setup.sh
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- overlays/caas-ci/files/osac-aap-secrets.env.example
- overlays/caas-ci/kustomization.yaml
- docs/network-backend.md
- overlays/caas-ci/files/osac-aap-configuration.env
- docs/aap-configuration.md
- overlays/development/files/osac-aap-configuration.env
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/setup.sh
- overlays/development/files/osac-aap-secrets.env.example
- README.md
| key="${key%%#*}" | ||
| key="${key// /}" | ||
| [[ -z "${key}" ]] && continue |
There was a problem hiding this comment.
Inline # comment is stripped from keys but kept in values.
Line 27 only strips # from the key portion. A line like NETRIS_USERNAME=admin # default will produce value=" admin # default" (with leading space), which then gets pushed into the ConfigMap verbatim. Either strip trailing inline comments from value as well, or document that inline comments aren't supported and only full-line # comments are.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/aap-configuration.sh` around lines 27 - 29, The code currently strips
inline comments only from the key (variable key) but leaves trailing inline
comments and leading spaces in the value; update the value processing in
scripts/aap-configuration.sh where key and value are parsed so that after
extracting value you first remove any inline “#” comment (e.g., value = value
before the first #) and then trim leading/trailing whitespace from value before
using it; reference the existing variables key and value in your change (mirror
the key handling: apply value="${value%%#*}" then apply a trim routine) so
ConfigMap entries don’t include the inline comment or stray spaces.
| echo "data:" > "${patch_file}" | ||
| for var in "${CM_VARS[@]}"; do | ||
| if [[ -n "${!var:-}" ]]; then | ||
| escaped="${!var//\'/\'\'}" | ||
| printf " %s: '%s'\n" "${var}" "${escaped}" >> "${patch_file}" | ||
| has_cm_overrides=true | ||
| fi | ||
| done |
There was a problem hiding this comment.
Multiline CM_VARS values produce invalid YAML.
The patch file uses single-quoted YAML scalars: printf " %s: '%s'\n" "${var}" "${escaped}". YAML single-quoted scalars may not contain raw newlines on the same logical line, so any CM_VAR value containing a newline (e.g., a multi-line NETRIS_RESOURCE_CLASS_MAP JSON, or anything copy-pasted with embedded newlines) will produce a malformed patch and oc patch --patch-file will fail. Using YAML block scalars (|) or JSON encoding (jq -Rs .) for the value would be safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/aap-configuration.sh` around lines 64 - 71, The current loop in the
script writes CM_VARS values as single-quoted YAML scalars which break on
embedded newlines; update the loop that writes to "${patch_file}" (the block
referencing CM_VARS, escaped, and has_cm_overrides) to detect multi-line values
and either emit a YAML block scalar (using a "|" followed by properly indented
lines) or JSON-encode the entire value (e.g., via jq -Rs .) and write the
encoded value as the field value; ensure you remove the current single-quote
escaping (escaped) for multi-line cases and set has_cm_overrides=true as before
so oc patch --patch-file receives valid YAML/JSON for multiline CM_VARS.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, omer-vishlitzky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Summary
Add Netris network backend support to the OSAC installer. Users
configure the
AAP automation backend via env files in their overlay —
osac-aap-configuration.envfor non-sensitive settings and
osac-aap-secrets.envfor credentials— making it
easy to switch between ESI and Netris without passing dozens of
environment
variables on the command line.
What changed
scripts/aap-configuration.sh(new): Readsosac-aap-configuration.envand, when present,
osac-aap-secrets.envfrom the overlay'sfiles/directory,then patches the
cluster-fulfillment-igConfigMap and Secret viaoc patch --type=merge. Shell env vars override file values for CIuse.
scripts/setup.sh: Callsaap-configuration.shafteroc apply -k,passing
INSTALLER_NAMESPACEandINSTALLER_KUSTOMIZE_OVERLAYtothe child
script.
overlays/{development,caas-ci}/files/osac-aap-configuration.env(new,
tracked): Non-sensitive settings pre-filled with Netris defaults for
the
development environment. Users edit to match their environment.
overlays/{development,caas-ci}/files/osac-aap-secrets.env.example(new,tracked): Template for sensitive credentials. Users copy to
osac-aap-secrets.env(gitignored) and fill in their values.overlays/{development,caas-ci,vmaas-ci}/kustomization.yaml:Added
configMapGeneratorforcluster-fulfillment-igwith ESI defaults.Comments
note that the script may override these values.
docs/aap-configuration.md(new): Documents the env filemechanism,
ConfigMap/Secret variable reference, plaintext secret requirement,
and SSH key
handling.
docs/network-backend.md(new): Documents Netris-specificvariables and the
NETRIS_RESOURCE_CLASS_MAPJSON format. Links back to the AAPconfiguration doc
for the general setup.
README.md: Added separate AAP Configuration and NetworkBackend
Configuration sections with links to the detailed docs. Updated
prerequisites
to list Netris as an alternative to ESI.
.gitignore: Added**/osac-aap-secrets.env.How to use
overlays/<project>/files/osac-aap-configuration.env— setNETWORK_CLASS=netrisand fill in connection detailsosac-aap-secrets.env.exampletoosac-aap-secrets.envand addcredentials (passwords, SSH keys, AWS keys)
./scripts/setup.shExisting ESI deployments are unaffected — without env files the script is a
no-op and the kustomize ESI defaults apply.
Summary by CodeRabbit
New Features
Documentation
Chores