Add medik8s-lib shared step to deduplicate common functions#79896
Add medik8s-lib shared step to deduplicate common functions#79896razo7 wants to merge 2 commits into
Conversation
Extract shared shell functions (log, run, set_proxy, resolve_commit_sha,
verify_fbc_image, wait_for_mcp_rollout, ensure_marketplace,
wait_for_catalogsource) and GitLab/Quay constants into a new medik8s-lib
step that writes them to ${SHARED_DIR}/medik8s-lib.sh.
Refactor medik8s-catalogsource and medik8s-operator-subscribe steps to
source the shared library, removing ~180 lines of duplicated code.
Follows the established step-registry pattern used by ovn-utils,
telcov10n-shared-functions, and openshift-microshift-includes.
Jira: RHWA-836
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extracts common medik8s helper functionality into a reusable shared library script, registers it as a CI step, and refactors two consumer scripts to source it instead of duplicating code. The library provides logging, proxy configuration, GitLab/Quay integration, MCP polling, and Kubernetes marketplace helpers that were previously inlined in individual step scripts. ChangesShared Library Extraction and Consumer Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7 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 |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
added
the
rehearsals-ack
![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 `@ci-operator/step-registry/medik8s/lib/medik8s-lib-commands.sh`:
- Around line 60-85: The current manifest check uses curl -sSf which treats any
non-2xx as failure and triggers the fallback; change the logic to explicitly
request and inspect the HTTP status code for the manifest URL (use curl -sS -o
/dev/null -w '%{http_code}' with the same retry/connect timeouts against
"https://quay.io/v2/${QUAY_REPO_PATH}/${image_name}/manifests/${FBC_COMMIT_SHA}"),
then only run the fallback tag lookup when the returned status is 404; for any
other non-200/404 status (timeouts, 5xx, network errors) log the full error and
exit with failure so we don’t silently replace FBC_COMMIT_SHA. Ensure you update
references in this block: the manifest check, the fallback_tag logic, and the
use of FBC_COMMIT_SHA/FBC_SHA_PINNED to reflect the new status-based branching.
- Around line 25-31: The set_proxy() function currently always returns 0 even if
sourcing "${SHARED_DIR}/proxy-conf.sh" fails; change it so that after attempting
to source the file (inside the [[ -f "${SHARED_DIR}/proxy-conf.sh" ]] block) you
capture the source command's exit status and return that status (or otherwise
return non-zero) instead of unconditionally returning 0—i.e., in set_proxy()
check the result of source "${SHARED_DIR}/proxy-conf.sh" and propagate failure
(return or exit) so callers see the error.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e030ed04-d63f-421d-8b6d-67ff3204a5a4
📒 Files selected for processing (6)
ci-operator/step-registry/medik8s/catalogsource/medik8s-catalogsource-commands.shci-operator/step-registry/medik8s/lib/OWNERSci-operator/step-registry/medik8s/lib/medik8s-lib-commands.shci-operator/step-registry/medik8s/lib/medik8s-lib-ref.metadata.jsonci-operator/step-registry/medik8s/lib/medik8s-lib-ref.yamlci-operator/step-registry/medik8s/operator-subscribe/medik8s-operator-subscribe-commands.sh
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| declare FBC_IMAGE_REPO="quay.io/redhat-user-workloads/rhwa-tenant/rhwa-fbc" | ||
| declare FBC_IMAGE_PREFIX="rhwa-fbc" | ||
| declare QUAY_REPO_PATH="redhat-user-workloads/rhwa-tenant/rhwa-fbc" | ||
| source "${SHARED_DIR}/medik8s-lib.sh" |
There was a problem hiding this comment.
CI blocker: this source line fails step-registry-shellcheck with SC1091 because shellcheck cannot follow runtime-generated paths. Add the shellcheck directive and an error guard so a missing lib fails with an actionable message instead of a cryptic "No such file or directory".
Fix:
# shellcheck source=/dev/null
source "${SHARED_DIR}/medik8s-lib.sh" || { echo "ERROR: medik8s-lib.sh not found. Did you include medik8s-lib ref before this step?" >&2; exit 1; }Same fix needed in medik8s-operator-subscribe-commands.sh:3.
| @@ -1,18 +1,12 @@ | |||
| #!/bin/bash | |||
| set -eu -o pipefail | |||
| source "${SHARED_DIR}/medik8s-lib.sh" | |||
There was a problem hiding this comment.
Same shellcheck CI blocker as in medik8s-catalogsource-commands.sh:3. Add # shellcheck source=/dev/null above this line and consider adding the error guard.
Fix:
# shellcheck source=/dev/null
source "${SHARED_DIR}/medik8s-lib.sh" || { echo "ERROR: medik8s-lib.sh not found. Did you include medik8s-lib ref before this step?" >&2; exit 1; }| @@ -0,0 +1,23 @@ | |||
| { | |||
There was a problem hiding this comment.
CI blocker: this file does not match the byte-for-byte output of make registry-metadata, causing the step-registry-metadata CI check to fail. Metadata JSON files in the step-registry must be generated, not hand-written.
Fix: Run make registry-metadata from the repo root and commit the regenerated file.
| run oc -n openshift-marketplace get pods -l "olm.catalogSource=$CATALOG_SOURCE_NAME" -o yaml | ||
| log "--- Marketplace events ---" | ||
| oc get events -n openshift-marketplace --sort-by='.lastTimestamp' 2>/dev/null | tail -30 || true | ||
| run oc get mcp,node |
There was a problem hiding this comment.
Regression: the extraction dropped a node-level image pull diagnostic block that was present in the original wait_for_catalogsource(). When the CatalogSource fails to become READY, this block identifies whether the node can actually pull the catalog image, which is critical for diagnosing image-pull failures.
Fix: Restore the following block before run oc get mcp,node:
local node_name
node_name=$(oc -n openshift-marketplace get pods -l "olm.catalogSource=$CATALOG_SOURCE_NAME" \
-o=jsonpath='{.items[0].spec.nodeName}' 2>/dev/null || true)
if [[ -n "$node_name" ]]; then
run oc debug "node/$node_name" -- chroot /host podman pull --authfile /var/lib/kubelet/config.json "${CATALOG_IMAGE}" || true
fi- Add shellcheck source=/dev/null directive and error guard to both consumer scripts (SC1091 CI blocker) - Remove trailing newline from metadata JSON to match make registry-metadata output (step-registry-metadata CI blocker) - Restore node-level image pull diagnostic block dropped during extraction of wait_for_catalogsource() - Fix set_proxy() to propagate source failures instead of masking them with unconditional return 0 - Check HTTP status code explicitly in verify_fbc_image() to distinguish 404 (fallback) from transient errors (fail fast) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Holding this PR until #79687 is merged (then we will rebase the PR with additional fixes) |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[REHEARSALNOTIFIER] Note: If this PR includes changes to step registry files ( Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@razo7: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
Summary
Extracts shared shell functions from medik8s step-registry steps into a
reusable
medik8s-libstep, following the established pattern used byovn-utils,
telcov10n-shared-functions,
and openshift-microshift-includes.
What changed
New step:
medik8s-lib— writes shared functions to${SHARED_DIR}/medik8s-lib.sh:log(),run(),set_proxy()— universal utilitiesresolve_commit_sha(),verify_fbc_image()— FBC/GitLab resolutionwait_for_mcp_rollout(),ensure_marketplace(),wait_for_catalogsource()— cluster operationsGITLAB_API,FBC_IMAGE_REPO, etc.)Refactored consumers:
medik8s-catalogsource— sources lib, removes ~165 lines of duplicated codemedik8s-operator-subscribe— sources lib, removes ~15 lines of duplicated codeWhy now
medik8s-disconnected-catalogsourcestep (Add medik8s disconnected CatalogSource step for air-gapped operator testing #79687) duplicates the same functionsUsage
Workflow configs must include
medik8s-libbefore other medik8s steps:Files
Jira: RHWA-836
Summary by CodeRabbit
This PR adds a reusable medik8s shared-step and refactors two medik8s steps to source it, reducing duplication in OpenShift CI step-registry configuration.
What changed (practical impact)
New step registry entry: ci-operator/step-registry/medik8s/lib
Consumers refactored to source the shared library:
Why / risk / timing
Notable commit fixes and review feedback addressed
Files of interest
Jira: RHWA-836