MGMT-24219: add cluster template support to prepare-fulfillment-service - #96
Conversation
Gate template-publishing on either INSTALLER_VM_TEMPLATE or INSTALLER_CLUSTER_TEMPLATE being set. Wait for clustertemplate using osac get clustertemplate when INSTALLER_CLUSTER_TEMPLATE is provided.
|
@omer-vishlitzky: This pull request references MGMT-24219 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 epic 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. |
WalkthroughThe pull request extends Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/prepare-fulfillment-service.sh (1)
44-58: ⚖️ Poor tradeoffConsider parallel waits when both templates are set.
When both
INSTALLER_VM_TEMPLATEandINSTALLER_CLUSTER_TEMPLATEare set, the current sequential waiting can take up to 600 seconds (300s + 300s). Since both templates are published by the same AAP job, waiting for them in parallel could reduce total wait time to approximately 300 seconds in CI scenarios.This is not a functional issue—the current implementation is correct—but could improve CI efficiency.
🤖 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 `@scripts/prepare-fulfillment-service.sh` around lines 44 - 58, When both INSTALLER_VM_TEMPLATE and INSTALLER_CLUSTER_TEMPLATE are set the script waits sequentially causing up to double timeout; change the logic around the two retry_until calls so they run in parallel (background the retry_until invocations and wait for both), ensure you still capture non-zero exit of either retry_until and propagate a single failure exit code, and keep the same messages for computeinstancetemplate and clustertemplate; reference the INSTALLER_VM_TEMPLATE and INSTALLER_CLUSTER_TEMPLATE checks and the retry_until invocation to locate where to background, wait, and handle errors.
🤖 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.
Nitpick comments:
In `@scripts/prepare-fulfillment-service.sh`:
- Around line 44-58: When both INSTALLER_VM_TEMPLATE and
INSTALLER_CLUSTER_TEMPLATE are set the script waits sequentially causing up to
double timeout; change the logic around the two retry_until calls so they run in
parallel (background the retry_until invocations and wait for both), ensure you
still capture non-zero exit of either retry_until and propagate a single failure
exit code, and keep the same messages for computeinstancetemplate and
clustertemplate; reference the INSTALLER_VM_TEMPLATE and
INSTALLER_CLUSTER_TEMPLATE checks and the retry_until invocation to locate where
to background, wait, and handle errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d69942d4-858f-4173-a1fb-d499235e0ab7
📒 Files selected for processing (1)
scripts/prepare-fulfillment-service.sh
|
[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 |
https://issues.redhat.com/browse/MGMT-24219
Adds INSTALLER_CLUSTER_TEMPLATE support so CaaS CI can trigger
template publishing and wait for cluster templates to appear.
or INSTALLER_CLUSTER_TEMPLATE being non-empty
osac get clustertemplate with the same retry_until pattern
Summary by CodeRabbit