Skip to content

OSAC-1338: Create upgrade playbook and migration framework#461

Merged
maorfr merged 10 commits into
mainfrom
OSAC-1338-upgrade-playbook
Jun 11, 2026
Merged

OSAC-1338: Create upgrade playbook and migration framework#461
maorfr merged 10 commits into
mainfrom
OSAC-1338-upgrade-playbook

Conversation

@maorfr

@maorfr maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR adds an upgrade framework to handle migrations from previous Enclave versions to the current version.

Changes

New files:

  • upgrade.sh - Entry point script following sync.sh patterns
  • playbooks/upgrade.yaml - Main orchestration playbook
  • playbooks/tasks/migrations.yaml - Migration coordinator
  • playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml - Reusable migration task

Initial migrations:

  • ODF operator subscription: core catalog source → redhat-operator-index-odf
  • LVMS operator subscription: core catalog source → redhat-operator-index-lvms

Background

Previous Enclave versions mirrored foundation plugin operators as part of the core catalog source. The plugin framework now creates dedicated catalog sources per plugin (introduced in #420, #456, #457). This migration ensures existing deployments are updated to use the new plugin-specific catalog sources.

Design

The upgrade framework is designed for extensibility:

  1. upgrade.sh - Minimal wrapper that follows sync.sh patterns:

    • getValue() function to extract config
    • step_done() helper with color-coded output
    • Timestamped logs in ${workingDir}/logs/${DSTAMP}-upgrade
  2. playbooks/upgrade.yaml - Clean orchestration:

    • Loads configuration via common/load-vars.yaml
    • Includes tasks/migrations.yaml
    • Extensible for future upgrade phases
  3. playbooks/tasks/migrations.yaml - Migration registry:

    • Lists all migrations
    • Conditionally includes based on enabled_plugins
    • Easy to add new migrations
  4. playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml - Reusable task:

    • Variable-driven (plugin_name, operator_name, operator_namespace)
    • Handles any foundation plugin migration
    • DRY principle - no duplicated code

Testing

The migration is:

  • Idempotent - safe to run multiple times
  • Conditional - only runs when plugins are enabled
  • Non-destructive - only patches subscription sources

Usage

bash sync.sh     # Sync content first
bash upgrade.sh  # Run upgrade migrations

Future Work

Additional foundation plugin migrations can be added by:

  1. Adding a new include_tasks block in playbooks/tasks/migrations.yaml
  2. Providing the plugin-specific variables (plugin_name, operator_name, operator_namespace)

Other upgrade phases (operator upgrades, validation) can be added as additional playbooks imported from upgrade.yaml.

Links

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added automated upgrade workflow for enclave deployments
    • Operator catalog sources automatically migrated to plugin-specific sources during upgrades

Add upgrade.yaml playbook and upgrade.sh script to handle migrations
from previous Enclave versions to the current version.

Framework structure:
- upgrade.sh: Entry point script following sync.sh patterns with
  getValue(), step_done(), and timestamped logging
- playbooks/upgrade.yaml: Main orchestration playbook that loads
  configuration and runs migrations
- playbooks/tasks/migrations.yaml: Migration coordinator that
  conditionally includes migrations based on enabled_plugins
- playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml:
  Reusable migration task for foundation plugins

Initial migrations:
- ODF operator subscription: core catalog → redhat-operator-index-odf
- LVMS operator subscription: core catalog → redhat-operator-index-lvms

Background: Previous Enclave versions mirrored foundation plugin
operators as part of the core catalog source. The plugin framework now
creates dedicated catalog sources per plugin (introduced in #420, #456,
#457). This migration ensures existing deployments are updated to use
the new plugin-specific catalog sources.

The migration is idempotent and only runs when the respective plugin
is enabled in the deployment configuration.

Assisted-by: Claude Code <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@maorfr, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 50 minutes and 19 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2aab6da7-b8e0-4371-8b70-67a59479e6e6

📥 Commits

Reviewing files that changed from the base of the PR and between f085edb and 2df5bef.

📒 Files selected for processing (3)
  • playbooks/tasks/migrations.yaml
  • playbooks/tasks/migrations/foundation_plugin_catalog_sources.yaml
  • playbooks/tasks/migrations/operator_catalog_source.yaml

Walkthrough

This PR introduces a comprehensive enclave upgrade workflow combining Bash orchestration, Ansible playbook dispatch, plugin discovery, and operator subscription migration. The upgrade.sh entry point drives ansible-playbook execution with logged output, while migrations.yaml recursively discovers foundation plugins and patches their operator Subscriptions' catalog sources from legacy to mirror-specific values on disconnected deployments only.

Changes

Upgrade Workflow and Subscription Migrations

Layer / File(s) Summary
Upgrade script entry point
upgrade.sh
Bash entrypoint initializes timestamped logging under workingDir/logs, extracts configuration from config/global.yaml via embedded Python + jq, and invokes ansible-playbook upgrade.yaml with ANSIBLE_LOG_PATH and strict error handling (set -euo pipefail).
Upgrade playbook and migration dispatcher
playbooks/upgrade.yaml, playbooks/tasks/migrations.yaml (header)
Localhost playbook disables fact gathering, configures Kubernetes retry/delay parameters, loads shared vars via common/load-vars.yaml, and dispatches migration logic by including the migrations task orchestrator.
Plugin discovery and descriptor read
playbooks/tasks/migrations.yaml (discovery/slurp)
Recursively discovers plugin.yaml files under ../plugins with controlled depth, then slurps each descriptor's base64-encoded YAML content; execution gated to disconnected deployments only.
Foundation plugin filtering and per-operator dispatch
playbooks/tasks/migrations.yaml (filter/loop)
Decodes slurped descriptors, filters to foundation-type plugins whose names exist in enabled_plugins, then loops over each foundation plugin's operators and includes the per-operator migration task with computed catalog source values derived from catalog_mirror; skips operators that already define source.
Subscription catalog source migration task
playbooks/tasks/migrations/operator_catalog_source.yaml
Queries Subscription by operator_name/namespace, conditionally patches spec.source to new_source only when source is defined, differs from new_source, and matches legacy_source; retries with backoff and emits debug status (not found, already migrated, or migrated from legacy to new).

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant upgrade.sh
  participant config/global.yaml
  participant playbooks/upgrade.yaml
  User->>upgrade.sh: invoke
  upgrade.sh->>config/global.yaml: getValue workingDir
  config/global.yaml-->>upgrade.sh: workingDir path
  upgrade.sh->>upgrade.sh: create timestamped log dir/file
  upgrade.sh->>upgrade.sh: write start timestamp
  upgrade.sh->>playbooks/upgrade.yaml: ansible-playbook with ANSIBLE_LOG_PATH
  playbooks/upgrade.yaml-->>upgrade.sh: execution complete
  upgrade.sh->>upgrade.sh: write "Done" completion timestamp
Loading
sequenceDiagram
  participant MigrationTask as migrations.yaml
  participant SubMigration as operator_catalog_source.yaml
  participant KubeAPI as Kubernetes API
  MigrationTask->>SubMigration: include task with operator_name,<br/>legacy_source, new_source
  SubMigration->>KubeAPI: query Subscription by name/namespace
  KubeAPI-->>SubMigration: Subscription resource or not found
  alt Subscription found and source needs update
    SubMigration->>KubeAPI: PATCH spec.source = new_source
    KubeAPI-->>SubMigration: patch applied (retry until success)
    SubMigration->>SubMigration: debug: migrated from legacy to new
  else Already on new_source or source undefined
    SubMigration->>SubMigration: debug: already migrated or skip
  else Subscription not found
    SubMigration->>SubMigration: debug: subscription not found
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • rh-ecosystem-edge/enclave#309: Updates default operator source derivation (e.g., cs-mirror-redhat-operators-*) for disconnected subscription creation; aligns with this PR's spec.source migration from legacy to mirror-specific catalog sources.

Suggested labels

deployment, operators, plugins

Suggested reviewers

  • rporres
  • eliorerz

🚀 ⚠️ Security Review Note: This PR modifies operator Subscription catalog sources on disconnected deployments. Risk Severity: Medium. Ensure that:

  • The catalog_mirror selection logic safely constructs mirror URLs and prevents injection of untrusted sources
  • The conditional patch only applies when legacy_source precisely matches the existing spec.source (currently enforced)
  • RBAC and audit logging capture all Subscription patch operations
  • Rollback procedures exist if a migration targets an unavailable or corrupted mirror
    📋 Subscription patches flow secure,
    Foundation plugins align and endure,
    Catalogs migrate with grace,
    Each operator finds its place,
    Enclave upgrades, clean and pure. 🔐
🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'OSAC-1338: Create upgrade playbook and migration framework' directly and specifically describes the main changes: adding an upgrade orchestration framework with playbooks and migration tasks.
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 detected in the PR files. All sensitive values use variable references and external configuration only.
No-Weak-Crypto ✅ Passed No weak cryptography (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto, or non-constant-time secret comparisons detected. Base64 is standard Ansible file transport.
No-Injection-Vectors ✅ Passed No injection vectors found. All YAML uses safe_load(), no eval/exec/os.system, no shell=True with untrusted input. Safe Ansible/Kubernetes APIs only.
Container-Privileges ✅ Passed PR adds only Ansible playbooks and shell scripts; no container/K8s manifests with security context definitions, privileged modes, or capabilities are present.
No-Sensitive-Data-In-Logs ✅ Passed Logging contains only safe data: operator names, catalog source names, and timestamps. No passwords, tokens, API keys, PII, or secrets are exposed.
Ai-Attribution ✅ Passed Commit f085edb properly attributes AI assistance using 'Assisted-by: Claude Code' trailer. No commits use Co-Authored-By for AI tools. Attribution is compliant with Red Hat standards.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch OSAC-1338-upgrade-playbook

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.

The catalog source name is constructed as {catalog_mirror}-{plugin_name},
where catalog_mirror can be either mirror_rh_operator_catalog
(mirror-redhat-operators) or mirror_certified_rh_operator_catalog
(mirror-certified-operators) depending on the plugin's catalog type.

This matches the catalog source naming in deploy_plugin.yaml:
- catalog_mirror: mirror-redhat-operators (default)
- catalog_mirror: mirror-certified-operators (when plugin.catalog == certified)

Both ODF and LVMS use the default redhat catalog, so they use
mirror-redhat-operators as the catalog_mirror prefix.

Assisted-by: Claude Code <noreply@anthropic.com>
@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@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: 4

🤖 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 `@playbooks/tasks/migrations.yaml`:
- Line 18: The when condition "'odf' in enabled_plugins" can fail if
enabled_plugins is undefined; update the conditional(s) in
playbooks/tasks/migrations.yaml (the when expressions referencing
enabled_plugins) to safely handle a missing variable by using the default filter
(e.g., check membership against enabled_plugins | default([])) so the play
remains rerunnable for older or partial configs; apply the same change to the
other occurrence referenced in the comment.

In `@playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml`:
- Around line 42-45: The current when clause uses
"r_plugin_subscription.resources[0].spec.source != catalog_mirror + "-" +
plugin_name" which causes the task to run for any non-matching/custom source;
change this to an equality check so the play only runs when the existing
subscription source exactly matches the legacy catalog (i.e. use
"r_plugin_subscription.resources[0].spec.source == catalog_mirror + "-" +
plugin_name"); make the same change for the other occurrence covering lines
46-56 so the patch is restricted to the legacy catalog source only.

In `@upgrade.sh`:
- Around line 11-12: The shell invocations in upgrade.sh are using unquoted
variable expansions (e.g. python -c '...'< $global_vars | jq -r $1 and similar
on lines mentioned), which can break on paths with spaces or glob characters;
update all occurrences to quote the expansions (use "$global_vars", and pass the
jq argument as "$1" or "${1}" where used, and similarly quote any other
variables on lines 16-17, 25 and 28-29) so file redirections and jq arguments
are protected from word-splitting and globbing.
- Around line 1-3: The script currently enables errexit and pipefail but is
missing nounset; update the top of upgrade.sh so it uses nounset (set -u) to
avoid silent failures from unset variables — replace the separate "set -o
pipefail" and duplicate "set -e" pattern with a single robust line like "set
-euo pipefail" after the shebang, ensuring you remove the redundant "set -e" so
the script enforces errexit, nounset and pipefail consistently.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 30116a0d-00a2-4f6f-80df-f610be885923

📥 Commits

Reviewing files that changed from the base of the PR and between b69cc60 and 3eee3df.

📒 Files selected for processing (4)
  • playbooks/tasks/migrations.yaml
  • playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml
  • playbooks/upgrade.yaml
  • upgrade.sh

Comment thread playbooks/tasks/migrations.yaml Outdated
Comment thread playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml Outdated
Comment thread upgrade.sh Outdated
Comment thread upgrade.sh Outdated
1. upgrade.sh: Add set -u (nounset) and quote all variable expansions
   - Changed from 'set -e' + 'set -o pipefail' to 'set -euo pipefail'
   - Quoted all variable expansions to handle paths with spaces

2. migrations.yaml: Add default([]) to enabled_plugins checks
   - Prevents failure if enabled_plugins is undefined
   - Makes the playbook rerunnable for older/partial configs

3. foundation_plugin_catalog_source.yaml: Add safety checks
   - Verify spec.source is defined before comparison
   - Only migrate subscriptions from known core/legacy catalogs
   - Pattern matches: redhat-operators, cs-*, or catalog_mirror prefix
   - Prevents accidental overwrite of custom catalog sources

Assisted-by: Claude Code <noreply@anthropic.com>
@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Claude Code: Addressed CodeRabbit review comments in ffd3cd4

Fixed:

  1. upgrade.sh: Added set -u (nounset) and quoted all variable expansions to handle paths with spaces
  2. migrations.yaml: Added default([]) to enabled_plugins checks to prevent failures on undefined variables
  3. foundation_plugin_catalog_source.yaml: Added safety checks to only migrate subscriptions from known core/legacy catalog patterns (redhat-operators, cs-*, or catalog_mirror prefix)

These changes make the upgrade framework more robust and prevent accidental overwrite of custom catalog sources.

maorfr added 2 commits June 9, 2026 13:07
Foundation plugin catalog source migrations only apply to disconnected
deployments where catalog sources are mirrored locally. Connected
deployments use upstream Red Hat catalogs directly and don't need
migration.

Added disconnected check to both ODF and LVMS migration tasks.

Assisted-by: Claude Code <noreply@anthropic.com>
The legacy catalog source is exactly catalog_mirror (e.g.,
"mirror-redhat-operators"), not a pattern. Changed from regex pattern
matching to exact equality check.

This ensures we only migrate subscriptions that are using the exact
legacy core catalog source, not custom or other catalog sources.

Assisted-by: Claude Code <noreply@anthropic.com>
@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Claude Code: Additional updates in f552922 and 3c22ab4

Changes:

  1. Restricted to disconnected mode (f552922): Migrations now only run when disconnected | default(true) | bool is true, since connected deployments use upstream Red Hat catalogs directly
  2. Fixed legacy catalog check (3c22ab4): Changed from regex pattern to exact equality check. The legacy source is exactly catalog_mirror (e.g., "mirror-redhat-operators"), ensuring we only migrate subscriptions using the exact legacy core catalog source

The migration now has proper guards:

  • Only runs in disconnected mode
  • Only runs when plugin is enabled
  • Only migrates subscriptions using the exact legacy catalog source

@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai please review again and approve, all your comments have been addressed.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch current state of all modified files from the PR
echo "=== upgrade.sh ==="
cat upgrade.sh

echo ""
echo "=== playbooks/upgrade.yaml ==="
cat playbooks/upgrade.yaml

echo ""
echo "=== playbooks/tasks/migrations.yaml ==="
cat playbooks/tasks/migrations.yaml

echo ""
echo "=== playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml ==="
cat playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml

Length of output: 5935



@maorfr I've reviewed the current state of all four files. Here's a security-focused summary of what was verified:

Risk level: LOW — all patches are non-destructive and well-guarded.

File Check Status
upgrade.sh set -euo pipefail (includes -u / nounset)
upgrade.sh All variable expansions quoted
migrations.yaml disconnected | default(true) | bool guard
migrations.yaml enabled_plugins | default([]) on both ODF & LVMS
foundation_plugin_catalog_source.yaml Subscription existence check before patching
foundation_plugin_catalog_source.yaml spec.source defined guard
foundation_plugin_catalog_source.yaml Idempotency: skips if already on target source
foundation_plugin_catalog_source.yaml Exact equality check — only migrates from catalog_mirror (legacy source), preventing accidental overwrite of custom sources

One minor non-blocking nit: logdir in upgrade.sh is defined but unused (mkdir -p "$(dirname "$log")" is used instead) — they resolve to the same path, so there's no impact.

All previously flagged issues have been addressed. Approving the PR.

[approve]

Replace hardcoded ODF and LVMS migration blocks with dynamic discovery
that automatically handles ALL foundation plugins.

Changes:
- Discover all foundation plugins using the same pattern as
  collect_core_plugin_operators.yaml
- Loop over each foundation plugin's operators using subelements()
- Automatically determine catalog_mirror based on plugin.catalog field:
  - redhat (default) → mirror_rh_operator_catalog
  - certified → mirror_certified_rh_operator_catalog
- Skip operators with explicit operator.source defined

Benefits:
- Extensible: New foundation plugins automatically get migrated without
  code changes
- Maintainable: Follows existing Enclave patterns for plugin discovery
- Safe: Multiple guards (disconnected mode, enabled plugins, no custom
  source)
- DRY: No hardcoded plugin names

Example migration flow for ODF, LVMS, VAST-CSI:
- odf/odf-operator: mirror-redhat-operators → mirror-redhat-operators-odf
- lvms/lvms-operator: mirror-redhat-operators → mirror-redhat-operators-lvms
- vast-csi/vast-csi-operator: mirror-certified-operators → mirror-certified-operators-vast-csi

Assisted-by: Claude Code <noreply@anthropic.com>
@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Claude Code: Generalized migration to handle ALL foundation plugins in d9a3337

Changes:
Replaced hardcoded ODF and LVMS migration blocks with dynamic discovery that automatically handles any foundation plugin.

Implementation:

  1. Plugin discovery (same pattern as collect_core_plugin_operators.yaml):

    • Finds all plugin.yaml files
    • Filters by type: foundation and name in enabled_plugins
  2. Dynamic migration using subelements() to iterate:

    • Loops over all foundation plugins and their operators
    • Auto-determines catalog_mirror based on plugin.catalog:
      • redhat (default) → mirror_rh_operator_catalog
      • certifiedmirror_certified_rh_operator_catalog
    • Skips operators with explicit operator.source defined

Example flow (ODF, LVMS, VAST-CSI enabled):

  • odf/odf-operator: mirror-redhat-operatorsmirror-redhat-operators-odf
  • lvms/lvms-operator: mirror-redhat-operatorsmirror-redhat-operators-lvms
  • vast-csi/vast-csi-operator: mirror-certified-operatorsmirror-certified-operators-vast-csi

Benefits:

  • ✅ Extensible: New foundation plugins automatically migrated without code changes
  • ✅ Safe: Multiple guards (disconnected mode, enabled plugins, no custom source)
  • ✅ Maintainable: Follows existing Enclave patterns
  • ✅ DRY: No hardcoded plugin names

@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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml (1)

51-61: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Debug message logic incorrect for undefined or custom subscription sources (Risk: Minor – informational output only).

Impact: The else clause (lines 58-61) assumes migration succeeded, but migration is skipped when:

  1. spec.source is undefined (line 47 guards migration, but line 59 accesses undefined spec.source → Jinja2 error)
  2. spec.source is a custom catalog (line 49: spec.source != catalog_mirror, migration skipped, but debug incorrectly reports "migrated from <custom> to ...")

This misleads operators during troubleshooting and may error in edge cases.

🛡️ Proposed fix to handle all subscription source states
 - name: Display {{ plugin_name | upper }} migration status
   ansible.builtin.debug:
     msg: >-
       {% if r_plugin_subscription.resources | length == 0 %}
       {{ plugin_name | upper }} subscription not found — no migration needed
+      {% elif r_plugin_subscription.resources[0].spec.source is not defined %}
+      {{ plugin_name | upper }} subscription spec.source not defined — skipped
       {% elif r_plugin_subscription.resources[0].spec.source == catalog_mirror + "-" + plugin_name %}
       {{ plugin_name | upper }} subscription already using plugin-specific catalog source — no migration needed
+      {% elif r_plugin_subscription.resources[0].spec.source == catalog_mirror %}
+      {{ plugin_name | upper }} subscription migrated from {{ catalog_mirror }}
+      to {{ catalog_mirror }}-{{ plugin_name }}
       {% else %}
-      {{ plugin_name | upper }} subscription migrated from {{ r_plugin_subscription.resources[0].spec.source }}
-      to {{ catalog_mirror }}-{{ plugin_name }}
+      {{ plugin_name | upper }} subscription uses custom source {{ r_plugin_subscription.resources[0].spec.source }} — skipped to preserve custom configuration
       {% endif %}
🤖 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 `@playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml` around
lines 51 - 61, The debug block assumes
r_plugin_subscription.resources[0].spec.source always exists and misreports
custom sources; update the Jinja conditionals around r_plugin_subscription,
r_plugin_subscription.resources[0].spec, and
r_plugin_subscription.resources[0].spec.source to explicitly handle three cases:
(1) no subscription resources -> keep "subscription not found — no migration
needed" using r_plugin_subscription.resources | length == 0, (2) spec.source is
undefined -> emit "subscription source undefined — migration skipped"
referencing r_plugin_subscription.resources[0].spec, and (3) spec.source defined
but not equal to catalog_mirror or plugin-specific -> emit "subscription uses
custom source {{ r_plugin_subscription.resources[0].spec.source }} — migration
skipped"; preserve the existing message only when spec.source equals
catalog_mirror + "-" + plugin_name (already using plugin-specific catalog) or
when migration actually occurred, using the symbols r_plugin_subscription,
plugin_name, and catalog_mirror to locate and adjust the template logic.
🤖 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.

Outside diff comments:
In `@playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml`:
- Around line 51-61: The debug block assumes
r_plugin_subscription.resources[0].spec.source always exists and misreports
custom sources; update the Jinja conditionals around r_plugin_subscription,
r_plugin_subscription.resources[0].spec, and
r_plugin_subscription.resources[0].spec.source to explicitly handle three cases:
(1) no subscription resources -> keep "subscription not found — no migration
needed" using r_plugin_subscription.resources | length == 0, (2) spec.source is
undefined -> emit "subscription source undefined — migration skipped"
referencing r_plugin_subscription.resources[0].spec, and (3) spec.source defined
but not equal to catalog_mirror or plugin-specific -> emit "subscription uses
custom source {{ r_plugin_subscription.resources[0].spec.source }} — migration
skipped"; preserve the existing message only when spec.source equals
catalog_mirror + "-" + plugin_name (already using plugin-specific catalog) or
when migration actually occurred, using the symbols r_plugin_subscription,
plugin_name, and catalog_mirror to locate and adjust the template logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fd01a1e1-c249-40c0-aa8e-e8831ea9e8bc

📥 Commits

Reviewing files that changed from the base of the PR and between 3eee3df and d9a3337.

📒 Files selected for processing (3)
  • playbooks/tasks/migrations.yaml
  • playbooks/tasks/migrations/foundation_plugin_catalog_source.yaml
  • upgrade.sh

Catalog sources are named cs-{catalog_mirror}-{plugin_name}-v4-20, not
just {catalog_mirror}-{plugin_name}.

Legacy source: cs-mirror-redhat-operators-v4-20
New source: cs-mirror-redhat-operators-odf-v4-20

This matches the naming in deploy_plugin.yaml line 191:
  default_operator_source: "cs-{{ catalog_mirror }}-v4-20"

Updated:
- New source pattern to include cs- prefix and -v4-20 suffix
- Legacy source check to exact match: cs-{catalog_mirror}-v4-20
- Documentation to reflect correct naming

Assisted-by: Claude Code <noreply@anthropic.com>
@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Claude Code: Fixed catalog source naming in 397112b

Correction: Catalog sources follow the pattern cs-{catalog_mirror}-{plugin_name}-v4-20, matching the default_operator_source pattern from deploy_plugin.yaml line 191.

Examples:

  • Legacy source: cs-mirror-redhat-operators-v4-20 (core catalog without plugin)
  • ODF new source: cs-mirror-redhat-operators-odf-v4-20
  • LVMS new source: cs-mirror-redhat-operators-lvms-v4-20
  • VAST-CSI new source: cs-mirror-certified-operators-vast-csi-v4-20

Migration now correctly:

  • Checks if source == cs-{catalog_mirror}-v4-20 (legacy)
  • Patches to cs-{catalog_mirror}-{plugin_name}-v4-20 (new)

Rename foundation_plugin_catalog_source.yaml to operator_catalog_source.yaml
and remove all plugin-specific references to make it a truly generic task.

Changes:
1. Renamed file (git history preserved):
   - foundation_plugin_catalog_source.yaml → operator_catalog_source.yaml

2. Made operator_catalog_source.yaml generic:
   - Removed plugin-specific variable names (plugin_name, catalog_mirror)
   - Added generic variables: legacy_source, new_source
   - Changed variable names: r_plugin_* → r_operator_*
   - Simplified header to reflect generic nature

3. Updated migrations.yaml to compute sources:
   - Computes legacy_source and new_source before calling the task
   - Passes pre-computed source strings to the generic task
   - Keeps all plugin-specific logic (discovery, filtering, catalog
     determination) in migrations.yaml

Benefits:
- Clear separation of concerns: plugin logic vs. generic patching
- operator_catalog_source.yaml is 100% reusable for any operator
  subscription migration
- More explicit: legacy_source and new_source are clearer than
  implicit string construction
- Better naming: file reflects what it does, not who calls it

Assisted-by: Claude Code <noreply@anthropic.com>
@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Claude Code: Refactored migration task in f085edb

Changes:
Renamed foundation_plugin_catalog_source.yamloperator_catalog_source.yaml and made it 100% generic.

What changed:

  1. Removed all plugin-specific references from the task file:

    • Variables: plugin_name, catalog_mirrorlegacy_source, new_source
    • Variable names: r_plugin_*r_operator_*
    • Header: Now describes a generic operator migration task
  2. Updated migrations.yaml to compute source strings:

    • Computes legacy_source and new_source before calling the task
    • All plugin-specific logic stays in migrations.yaml

Benefits:

  • ✅ Clear separation: plugin logic in migrations.yaml, generic patching in operator_catalog_source.yaml
  • ✅ Reusable for any operator subscription migration (not just foundation plugins)
  • ✅ More explicit: legacy_source and new_source are clearer than implicit construction
  • ✅ Better naming: file reflects what it does, not who calls it

Git history preserved with git mv.

@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 `@playbooks/tasks/migrations/operator_catalog_source.yaml`:
- Around line 46-56: The debug message currently prints "migrated" whenever
r_operator_subscription.resources[0].spec.source != new_source, which can
produce false positives when a patch was blocked; change the conditional logic
so the "migrated" message is only emitted if the subscription source actually
changed to new_source (i.e., current source == new_source) or if you have an
explicit successful-patch indicator (e.g., a result variable like
r_operator_patch.changed is true); add a final branch that reports "migration
not performed/patch blocked — current source is {{
r_operator_subscription.resources[0].spec.source }}" for cases where the source
is neither new_source nor migrated (for example custom sources or
legacy_source), referencing operator_name,
r_operator_subscription.resources[0].spec.source, new_source and the patch
result variable when present.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 33469899-22c7-4d1b-bb38-8ec574fb38a4

📥 Commits

Reviewing files that changed from the base of the PR and between d9a3337 and f085edb.

📒 Files selected for processing (2)
  • playbooks/tasks/migrations.yaml
  • playbooks/tasks/migrations/operator_catalog_source.yaml

Comment thread playbooks/tasks/migrations/operator_catalog_source.yaml
Move foundation plugin catalog source migration logic from migrations.yaml
into its own dedicated file for better organization.

Changes:
- Created foundation_plugin_catalog_sources.yaml with:
  - Plugin discovery (find, load, filter foundation plugins)
  - Catalog mirror determination (redhat vs certified)
  - Operator subscription migration loop

- Simplified migrations.yaml to just:
  - Include foundation_plugin_catalog_sources.yaml

Benefits:
- migrations.yaml becomes a clean registry/index of all migrations
- Each migration is self-contained in its own file
- Easier to add new migrations (just create new file and include it)
- Better organization as migrations grow

File structure:
  playbooks/tasks/migrations/
  ├── foundation_plugin_catalog_sources.yaml  (specific migration)
  ├── operator_catalog_source.yaml            (generic utility)
  └── (future migrations can be added here)

Assisted-by: Claude Code <noreply@anthropic.com>
@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Claude Code: Extracted migration to separate file in d4a18f6

Changes:
Moved foundation plugin catalog source migration logic into its own dedicated file.

New structure:

playbooks/tasks/migrations/
├── foundation_plugin_catalog_sources.yaml  (specific migration logic)
├── operator_catalog_source.yaml            (generic utility task)
└── (future migrations can be added here)

migrations.yaml is now a clean registry:

- name: Foundation plugin catalog source migration
  ansible.builtin.include_tasks:
    file: migrations/foundation_plugin_catalog_sources.yaml

Benefits:

  • migrations.yaml is a clean index of all migrations
  • ✅ Each migration is self-contained in its own file
  • ✅ Easy to add new migrations (create file + include it)
  • ✅ Better organization as migrations grow

Perfect separation of concerns:

  • migrations.yaml - Registry/index
  • foundation_plugin_catalog_sources.yaml - Specific migration logic
  • operator_catalog_source.yaml - Generic utility task

1. Move disconnected condition to migrations.yaml:
   - Apply condition at the include level in migrations.yaml
   - Remove redundant disconnected checks from foundation_plugin_catalog_sources.yaml
   - Cleaner control flow - condition in one place

2. Fix debug message logic in operator_catalog_source.yaml:
   - Only show "migrated" when r_operator_patch.changed is true
   - Add branch for custom catalog sources (not legacy_source)
   - Add final catch-all branch for other cases
   - Prevents false positives when patch was blocked

Addresses CodeRabbit review comments.

Assisted-by: Claude Code <noreply@anthropic.com>
@maorfr

maorfr commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Claude Code: Addressed CodeRabbit review in 2df5bef

Changes:

  1. Moved disconnected condition to migrations.yaml:

    • Applied when: disconnected | default(true) | bool at the include level
    • Removed redundant disconnected checks from foundation_plugin_catalog_sources.yaml
    • Cleaner control flow - condition enforced in one place
  2. Fixed debug message logic in operator_catalog_source.yaml:

    • Only shows "migrated" when r_operator_patch.changed is true
    • Added branch for custom catalog sources (when source ≠ legacy_source)
    • Added final catch-all for other edge cases
    • Prevents false positives when patch was blocked

Debug message flow:

  1. Subscription not found → "no migration needed"
  2. Already using new source → "already using new catalog source"
  3. Patch succeeded → "migrated from X to Y" ✅
  4. Custom source detected → "has custom catalog source (X) — migration skipped"
  5. Catch-all → "migration not performed — current source: X"

Addresses all outstanding CodeRabbit review comments.

@maorfr

maorfr commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

e2e connected will be fixed in #469

@maorfr maorfr added the rc-ok label to indicate a PR is ok to arrive to main branch while preparing a release candidate label Jun 10, 2026
@maorfr

maorfr commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

post merge reviews are welcome

@maorfr maorfr merged commit d526a27 into main Jun 11, 2026
21 of 22 checks passed
@maorfr maorfr deleted the OSAC-1338-upgrade-playbook branch June 11, 2026 14:08

@rporres rporres left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So this covers the need of amending operators catalog sources, that looks great.

I like a lot the idea of migrations.

I think migrations should be somehow bound to a from/destination version to avoid testing hell to confirm the need for them to be perfectly idempotent.

A few comments here and there, nothing very important. This is a great starting point

Comment thread upgrade.sh
set -euo pipefail

# Upgrade script for Red Hat Sovereign Enclave
# Run sync.sh BEFORE running this script to ensure content is synchronized

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be great to have something that checks this...

Comment thread upgrade.sh
Comment on lines +9 to +17
getValue(){
python -c 'import sys, yaml, json; print(json.dumps(yaml.safe_load(sys.stdin)))' < "$global_vars" \
| jq -r "$1"
}

step_done(){
echo -e "\e[38;5;10m Done...\033[0m" | tee -a "${log}"
date | tee -a "${log}"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can we avoid duplicating these from bootstrap.sh?

Comment on lines +7 to +10
# Background: Previous Enclave versions mirrored foundation plugin operators
# as part of the core catalog source (e.g., cs-mirror-redhat-operators-v4-20).
# The plugin framework now creates dedicated catalog sources per plugin (e.g.,
# cs-mirror-redhat-operators-odf-v4-20 for ODF).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So this migration is to be applied if we're migrating from 0.1.0, right? It shouldn't be run at any other point although it looks idempotent, right?

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

Labels

rc-ok label to indicate a PR is ok to arrive to main branch while preparing a release candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants