Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.

bump submodules + fix refresh script reliability - #163

Merged
openshift-merge-bot[bot] merged 1 commit into
osac-project:mainfrom
omer-vishlitzky:fix-refresh-timeouts
May 25, 2026
Merged

openshift-merge-bot[bot] merged 1 commit into
osac-project:mainfrom
omer-vishlitzky:fix-refresh-timeouts

Conversation

@omer-vishlitzky

Copy link
Copy Markdown
Contributor

Summary

  • Bump all submodules to latest main
  • Fix prepare-aap.sh: capture curl response before jq to diagnose non-JSON gateway errors
  • Fix refresh-after-snapshot.sh: bump keycloak-set-passwords timeout from 120s to 300s (job needs ~3min for Keycloak startup)

Test plan

  • CI e2e-vmaas passes
  • kustomize-build-all passes

Bump all submodules to latest main.

prepare-aap.sh: capture curl response before piping to jq so
non-JSON responses from the AAP gateway are logged instead of
producing a silent jq parse error.

refresh-after-snapshot.sh: bump keycloak-set-passwords job
timeout from 120s to 300s. The job needs Keycloak to fully
start after snapshot boot which can take up to 3 minutes.
@openshift-ci
openshift-ci Bot requested review from eranco74 and tzvatot May 25, 2026 12:20
@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown

Review Change Stack

Summary by CodeRabbit

  • Chores

    • Updated container image tags and component versions for fulfillment service, AAP, and OSAC operator.
    • Updated configuration overlays with new AAP image references.
  • Bug Fixes

    • Enhanced error handling in deployment scripts with improved response parsing and diagnostic messages.
    • Increased job timeout to improve reliability of password initialization process.

Walkthrough

This pull request coordinates version updates across the OSAC project by advancing three component submodules (osac-aap, osac-fulfillment-service, osac-operator) to new commits and updating their corresponding image tag overrides in the base kustomization.yaml. These changes are then propagated through three environment overlay configurations (caas-ci, osac-integration, vmaas-ci) to ensure AAP build references remain synchronized. Additionally, two shell scripts are hardened: prepare-aap.sh gains explicit error handling for token creation responses, and refresh-after-snapshot.sh increases a Kubernetes Job wait timeout from 120s to 300s.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main changes: bumping submodules and fixing script reliability issues.
Description check ✅ Passed The description directly relates to the changeset, providing clear summaries of submodule bumps and specific fixes to two shell scripts.
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.

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


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

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@scripts/prepare-aap.sh`:
- Around line 22-26: The curl call that sets AAP_RESPONSE in prepare-aap.sh can
hang indefinitely; update the curl invocation (the command assigning
AAP_RESPONSE) to include explicit timeouts (e.g., --connect-timeout 10 and
--max-time 30 or values chosen by your policy) so connection and total request
time are bounded, and ensure these flags are added alongside the existing -sk -X
POST -u ... -H ... -d ... "${AAP_URL}/api/gateway/v1/tokens/" invocation.
🪄 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: Enterprise

Run ID: 84c38ef4-72e3-4c0d-83d0-8c826432f8b4

📥 Commits

Reviewing files that changed from the base of the PR and between c6de027 and 6d05327.

📒 Files selected for processing (9)
  • base/kustomization.yaml
  • base/osac-aap
  • base/osac-fulfillment-service
  • base/osac-operator
  • overlays/caas-ci/kustomization.yaml
  • overlays/osac-integration/kustomization.yaml
  • overlays/vmaas-ci/kustomization.yaml
  • scripts/prepare-aap.sh
  • scripts/refresh-after-snapshot.sh

Comment thread scripts/prepare-aap.sh
Comment on lines +22 to +26
AAP_RESPONSE=$(curl -sk -X POST \
-u "admin:${AAP_ADMIN_PASSWORD}" \
-H "Content-Type: application/json" \
-d '{"description": "osac-operator", "scope": "write"}' \
"${AAP_URL}/api/gateway/v1/tokens/" | jq -r '.token')
"${AAP_URL}/api/gateway/v1/tokens/")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add explicit curl timeouts to prevent indefinite hangs

Line 22 performs a remote call without --connect-timeout/--max-time. If the gateway or network stalls, this script can block indefinitely and break refresh automation reliability.

Suggested patch
-AAP_RESPONSE=$(curl -sk -X POST \
+AAP_RESPONSE=$(curl -skS --connect-timeout 10 --max-time 60 -X POST \
     -u "admin:${AAP_ADMIN_PASSWORD}" \
     -H "Content-Type: application/json" \
     -d '{"description": "osac-operator", "scope": "write"}' \
     "${AAP_URL}/api/gateway/v1/tokens/")

As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

📝 Committable suggestion

‼️ 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.

Suggested change
AAP_RESPONSE=$(curl -sk -X POST \
-u "admin:${AAP_ADMIN_PASSWORD}" \
-H "Content-Type: application/json" \
-d '{"description": "osac-operator", "scope": "write"}' \
"${AAP_URL}/api/gateway/v1/tokens/" | jq -r '.token')
"${AAP_URL}/api/gateway/v1/tokens/")
AAP_RESPONSE=$(curl -skS --connect-timeout 10 --max-time 60 -X POST \
-u "admin:${AAP_ADMIN_PASSWORD}" \
-H "Content-Type: application/json" \
-d '{"description": "osac-operator", "scope": "write"}' \
"${AAP_URL}/api/gateway/v1/tokens/")
🤖 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-aap.sh` around lines 22 - 26, The curl call that sets
AAP_RESPONSE in prepare-aap.sh can hang indefinitely; update the curl invocation
(the command assigning AAP_RESPONSE) to include explicit timeouts (e.g.,
--connect-timeout 10 and --max-time 30 or values chosen by your policy) so
connection and total request time are bounded, and ensure these flags are added
alongside the existing -sk -X POST -u ... -H ... -d ...
"${AAP_URL}/api/gateway/v1/tokens/" invocation.

@openshift-ci

openshift-ci Bot commented May 25, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:
  • OWNERS [danmanor,omer-vishlitzky]

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

This was referenced May 25, 2026
@openshift-merge-bot
openshift-merge-bot Bot merged commit dbea781 into osac-project:main May 25, 2026
9 checks passed
@openshift-ci

openshift-ci Bot commented May 25, 2026

Copy link
Copy Markdown

@omer-vishlitzky: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-vmaas 6d05327 link unknown /test e2e-vmaas

Full PR test history. Your PR dashboard.

Details

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 understand the commands that are listed here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants