OSAC-1550: rename sshKey to sshPublicKey in AAP template parameters

[mennyaboush]

Summary

• Rename the AAP template parameter key from sshKey to sshPublicKey in the BareMetalInstance reconciler
• Update all corresponding test assertions and descriptions

Follows up on the review comment from PR #738: #738 (comment)

Test plan

• ginkgo run internal/controllers/baremetalinstance — 36/36 tests pass

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Corrected the SSH credential template parameter name for BareMetalInstance to use sshPublicKey instead of sshKey, ensuring public keys are passed under the expected parameter key.

• Tests

  • Updated BareMetalInstance test cases to assert sshPublicKey is used and to verify related template parameter behavior (including user data presence/absence).

[openshift-ci-robot]

@mennyaboush: This pull request references OSAC-1550 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.

Details

In response to this:

Summary

• Rename the AAP template parameter key from sshKey to sshPublicKey in the BareMetalInstance reconciler
• Update all corresponding test assertions and descriptions

Follows up on the review comment from PR #738: #738 (comment)

Test plan

• ginkgo run internal/controllers/baremetalinstance — 36/36 tests pass

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.

[coderabbitai]

Walkthrough

The SSH template parameter key changes from sshKey to sshPublicKey in the reconciler function. Tests update their setup, descriptions, and assertions to match the new key in both template-parameter cases.

Changes

SSH Template Parameter Key Rename

Layer / File(s)	Summary
SSH key rename in template parameters internal/controllers/baremetalinstance/baremetalinstance_reconciler_function.go, internal/controllers/baremetalinstance/baremetalinstance_reconciler_function_test.go	The template parameters map stores SshPublicKey under sshPublicKey instead of sshKey. Test descriptions, spec wiring, and assertions are updated for both the user-data and no-user-data cases.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

• osac-project/fulfillment-service#738: Directly overlaps with the same reconciler function and tests, changing how SSH credential data is mapped into TemplateParameters.
• osac-project/fulfillment-service#741: Also updates mutateBMI’s SSH-related TemplateParameters behavior in the same area of code.

Suggested labels

lgtm

Suggested reviewers

• trewest
• adriengentil

Poem

One key was named, then named anew,
sshPublicKey now shines through.
Tests nod along, both paths agree,
A tiny rename sails clean and free. 🔑

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: renaming the AAP template parameter from sshKey to sshPublicKey.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets were introduced; the change only renames a template key and the test uses a public SSH key fixture, not a private credential.
No-Weak-Crypto	✅ Passed	PR contains no cryptographic operations, weak crypto usage, or custom crypto implementations. Changes are purely a parameter key rename from 'sshKey' to 'sshPublicKey' with corresponding test updates.
No-Injection-Vectors	✅ Passed	The PR only renames a template key and marshals a string map; no SQL/shell/eval/YAML/HTML injection sinks were added.
Container-Privileges	✅ Passed	Touched files only change template-key logic/tests; manifest scan found no privileged:true, hostPID/Network/IPC, SYS_ADMIN, or allowPrivilegeEscalation:true.
No-Sensitive-Data-In-Logs	✅ Passed	The PR only renames a template parameter and updates tests; no logging of secrets/PII was added, and existing logs only include object namespace/name/IDs.
Ai-Attribution	✅ Passed	HEAD includes Assisted-by: Claude Code <noreply@anthropic.com> and no Co-Authored-By trailer was found.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[adriengentil]

/hold

let's unhold once osac-project/osac-aap#368 is ready

[mennyaboush]

/retest

[adriengentil]

/retest

[mennyaboush]

CI failures are unrelated to this PR:

• Unit tests: duplicate migration file: 60_create_external_ip_tables.up.sql — migration conflict on main
• Integration tests: failed at /etc/hosts setup step (CI infra issue)
• Prow CI (ci/prow/unit, ci/prow/images) passed

[coderabbitai]

Actionable comments posted: 1

🤖 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
`@internal/controllers/baremetalinstance/baremetalinstance_reconciler_function_test.go`:
- Around line 224-225: The BareMetalInstance reconciler test currently only
verifies the presence of sshPublicKey, so it would still pass if the legacy
sshKey parameter were emitted alongside it. Update the relevant assertions in
baremetalinstance_reconciler_function_test to explicitly check that params does
not contain sshKey in both scenarios, using the same params map setup around the
baremetalinstance reconciler function expectations.

🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 21af8e70-3838-4f4f-b8d2-dc03ed0391c7

📥 Commits

Reviewing files that changed from the base of the PR and between 1a3bf03 and 6a8267d.

📒 Files selected for processing (2)

• internal/controllers/baremetalinstance/baremetalinstance_reconciler_function.go
• internal/controllers/baremetalinstance/baremetalinstance_reconciler_function_test.go

[omer-vishlitzky]

💀 CI Triage: broken_main | Category: BOOT

Root cause: The main branch was broken due to a database migration number collision (migration 60) between PR #706 and PR #740, causing the fulfillment-grpc-server to crash at startup.

Explanation: The boot step failed during the refresh phase because the fulfillment-console-proxy deployment timed out waiting to roll out. The proxy depends on the fulfillment-grpc-server, which was crashing at startup. The fulfillment-grpc-server pod logs show a fatal error: failed to init driver with path migrations: duplicate migration file: 60_create_external_ip_tables.up.sql. This occurred because two PRs recently merged into the main branch using the same migration number (60): PR #706 added 60_add_projects_immutable_trigger.up.sql and PR #740 added 60_create_external_ip_tables.up.sql. This collision broke the main branch. PR #761 was subsequently merged to fix this by renaming the migration to 61, but this test run occurred before the fix was merged.

Evidence:

build-log.txt:

ERROR: command failed (exit 1): oc rollout status deploy/fulfillment-console-proxy -n osac-e2e-ci --timeout=360s

pod-fulfillment-grpc-server-db45d958d-msx7g-grpc-server.log:

failed to init driver with path migrations: duplicate migration file: 60_create_external_ip_tables.up.sql

Suggestion: The issue has already been fixed on the main branch by PR #761. Rebase the PR on the latest main branch and retest.

---

Prow job | Build 2069681447496585216 | 🤖 triagent

_{For deeper investigation, use the /osac-debug-e2e skill with this build ID.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil, mennyaboush

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [adriengentil]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[adriengentil]

/unhold

[omer-vishlitzky]

💀 CI Triage: broken_main | Category: BOOT

Root cause: Helm upgrade fails because changing OSAC_AAP_TOKEN from a literal value to valueFrom causes a strategic merge patch conflict with the existing deployment in the snapshot.

Explanation: The main branch is broken. PR #328 in osac-installer and PR #316 in osac-operator changed the OSAC_AAP_TOKEN environment variable in the osac-operator deployment from a literal value to a valueFrom: secretKeyRef. The CI boot step uses a pre-built cluster snapshot (vmaas-helm) that contains the older version of the deployment with the literal value set. During the refresh phase, refresh-after-snapshot.py runs helm upgrade to apply the latest manifests. Helm uses a strategic merge patch to update the deployment, which merges the new valueFrom field with the existing value field. Kubernetes rejects the patched Deployment because an environment variable cannot have both value and valueFrom specified simultaneously, causing the boot step to fail for all PRs.

Evidence:

build-log.txt:

Error: UPGRADE FAILED: cannot patch "osac-operator" with kind Deployment: Deployment.apps "osac-operator" is invalid: spec.template.spec.containers[0].env[1].valueFrom: Invalid value: "": may not be specified when `value` is not empty

Suggestion: Fix the Helm chart in osac-operator to explicitly set value: null when valueFrom is used, or update refresh-after-snapshot.py to delete the osac-operator deployment before running helm upgrade, or rebuild the vmaas-helm snapshot.

---

Prow job | Build 2069839900894564352 | 🤖 triagent

_{For deeper investigation, use the /osac-debug-e2e skill with this build ID.}

[omer-vishlitzky]

/retest