Repository navigation
OSAC-3519: Add bare-metal-fulfillment-operator to e2e-vmaas/e2e-caas components arrays - #55
Conversation
|
@eliorerz: This pull request references OSAC-3519 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe CaaS and VMaaS full-install workflows now build ChangesE2E workflow image builds
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ 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 |
![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
🤖 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 @.github/workflows/e2e-caas-full-install.yml:
- Around line 59-61: Update the comments in
.github/workflows/e2e-caas-full-install.yml lines 59-61 and
.github/workflows/e2e-vmaas-full-install.yml lines 59-63 to distinguish image
building from deployment: state that enabled components are installed together,
while bare-metal-fulfillment-operator (BMF) is built only and not deployed
because bmf.enabled is false.
In @.github/workflows/e2e-vmaas-full-install.yml:
- Around line 71-73: Update the comment near the VMaaS component build
configuration to remove or narrow the claim that all four components build in
every e2e flow; describe the expectation only for this VMaaS flow while
preserving the note about revisiting BMF support.
🪄 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: Pro Plus
Run ID: 37600280-0877-4338-bb4f-d3a3cbfa19dc
📒 Files selected for processing (2)
.github/workflows/e2e-caas-full-install.yml.github/workflows/e2e-vmaas-full-install.yml
| # built from source for consistency with the other 3 components (all 4 | ||
| # should build from the PR in every e2e flow, even where one isn't | ||
| # currently deployed) -- revisit if vmaas ever needs BMF enabled. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Limit the “every e2e flow” claim.
.github/workflows/e2e-caas-full-install.yml Lines 66-67 explicitly exclude osac-aap. Therefore, all four components do not build in every e2e flow. Scope this statement to the VMaaS flow or remove the cross-flow 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 @.github/workflows/e2e-vmaas-full-install.yml around lines 71 - 73, Update
the comment near the VMaaS component build configuration to remove or narrow the
claim that all four components build in every e2e flow; describe the expectation
only for this VMaaS flow while preserving the note about revisiting BMF support.
Verification resultse2e-caas: https://github.com/osac-project/osac/actions/runs/30673628729
e2e-vmaas: https://github.com/osac-project/osac/actions/runs/30673628790
Both flows confirm the same evidence standard as PR #51 (bmaas): BMF genuinely builds from this PR's source and gets substituted into the installer, even though Ready for review. Not merging this myself. |
|
Update on the two flaky-looking failures mentioned above: dug further and both turned out to be known, already-tracked issues, not generic flakiness:
Neither is caused by this PR -- BMF's build/substitution succeeded in both runs before either unrelated failure occurred -- but wanted to correct the record rather than leave "just a flake" as the final word. |
…components arrays
2746deb to
a9cd15f
Compare
…-latest MGMT-23507: Update submodules to latest main
OSAC-4141: add end-to-end Metal3 installation documentation
OSAC-1960: Add GitHub Release creation to publish-charts workflow
This branch was previously deployed
2 participants
Summary
Final slice of OSAC-3519 -- extends PR #51's bmaas fix (merged) to the other two e2e flows. Same change, same imageKey, same containerfile path:
e2e-vmaas-full-install.yml: added bare-metal-fulfillment-operator (BMF) as a 4thcomponentsentry (alongside fulfillment-service, osac-operator, osac-aap).e2e-caas-full-install.yml: added BMF as a 3rdcomponentsentry. Note this file's own components array only has 2 entries today (no osac-aap) -- that's a pre-existing scope decision for this flow unrelated to BMF, left untouched.Both use
containerfile: "bare-metal-fulfillment-operator/Containerfile",imageKey: "bmf.image.repository"(nobuildType, same as fulfillment-service/osac-operator's entries), matching what was verified end-to-end in PR #51.Important: this is currently a no-op for both of these flows specifically
Re-confirmed against osac-installer's actual current values files before writing anything:
values/vmaas-ci/values.yaml:bmf.enabled: falsevalues/caas-ci/values.yaml:bmf.enabled: falseBMF's chart isn't deployed at all in either flow (unlike
values/bmaas-ci/values.yaml, where it'strue), so thebmf.image.repositoryoverride has nothing to apply to -- no running BMF pod exists in these clusters for the freshly-built image to reach. This is not a bug in this change, just a real, currently-inert side effect worth being explicit about rather than silent.Added anyway, for the same reason the other 3 components are built from source in every flow regardless of what each flow exercises: consistency (all 4 mono-repo components build from the PR everywhere), and it costs nothing beyond one extra image build per run. If vmaas/caas ever need BMF enabled, this wiring is already in place and will start actually mattering with no further change needed here.
Validation
YAML parse +
yamllint -c .yamllint.yaml --strictclean on both files. Also parsed the embeddedcomponentsJSON strings directly to confirm 4 entries (vmaas) / 3 entries (caas) with the expected keys.Verification plan
Will trigger real e2e-vmaas and e2e-caas runs and confirm from the logs that BMF builds from source (same evidence standard as PR #51 -- component build log entry, submodule replacement, image load), even though the override itself won't be exercised by a running BMF pod in either flow. Will report back with run links before this is ready to merge.
Not merging this myself. Once this merges, OSAC-3519 is fully done (all 3 e2e flows) and I'll close the ticket.
Summary by CodeRabbit