OSAC-1739: extend mono-repo tooling to osac-csi-driver cutover - #183
Merged
openshift-merge-bot[bot] merged 3 commits intoAug 3, 2026
Merged
Conversation
Fourth increment under OSAC-1739. osac-csi-driver cut over from a git submodule to a full osac-project/osac subdirectory (OSAC-3530), leaving it the last standalone/submodule component still described that way across osac-workspace's docs, scripts, and skills. Applies the same treatment already used for the other 5 merged components (fulfillment-service, osac-operator, osac-aap, osac-installer, bare-metal-fulfillment-operator): - bootstrap.sh: remove osac-csi-driver from REPOS, add it to MERGED_COMPONENTS so quarantine_merged_component() picks up any pre-existing standalone clone automatically. - AGENTS.md / README.md: add osac/osac-csi-driver's Repository Structure and Build/Test rows (previously undocumented even pre-merge), extend the merge-history note and Cross-Component Changes list, and rewrite the Deployment Coordination bullet to drop the stale "one remaining git submodule" framing (zero submodules remain in osac) while flagging that sync-image-tags.sh doesn't cover its image tag yet -- a real upstream gap, not papered over. - .claude/rules/cross-repo-workflow.md: add its CLAUDE.md routing row (with a "no CLAUDE.md yet" caveat, since none exists upstream) and bump "all five" -> "all six" component counts. - .skillsaw.yaml / .gitignore: fold osac-csi-driver into the same migration-window exclude/ignore blocks as the other 5 components. - skills/create-pr/SKILL.md: extend the mono-repo detection regex and File Classification table, and add an osac-csi-driver validation block. Full-branch self-review caught two gaps here, both fixed: the new block was missing the make fmt && git diff --exit-code drift gate every other Go component's block runs first (its Makefile has build/test silently depend on fmt), and the osac-installer block still referenced git submodule update --init --recursive, which this change's own "zero submodules remain" claim made stale. - reference/STRUCTURE.md / CONVENTIONS.md: hand-corrected (no repo-intel regeneration tool exists) to nest osac-csi-driver under osac/ and fold it into the sync-image-tags.sh coverage row instead of a standalone submodule row. While in STRUCTURE.md, discovered none of the other 5 mono-repo components were correctly nested under an osac/ node either -- restructured the full tree diagram for consistency, not just osac-csi-driver's entry. Non-goals (left untouched, tracked elsewhere): skills/osac-release/ (OSAC-3396), fixing sync-image-tags.sh or adding AGENTS.md/CLAUDE.md to osac/osac-csi-driver/ upstream (osac repo gaps, not osac-workspace bugs), and archiving the old standalone osac-csi-driver repo (OSAC-1737). Validated: make skillsaw clean (Grade A, 0 errors/warnings); live end-to-end bootstrap.sh --no-fork run against this machine's real standalone osac-csi-driver clone confirmed quarantine + idempotency + no regression to the 5 prior components; shellcheck clean (no new findings); independent full-branch self-review PASS after fixes. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
@tchughesiv: This pull request references OSAC-1739 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: tchughesiv 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 |
WalkthroughChangesThe PR integrates CSI driver monorepo integration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (9 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: 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 `@AGENTS.md`:
- Around line 44-54: Update the osac/osac-csi-driver row in the component table
to reference its README.md as the documentation fallback, since that component
has no AGENTS.md or CLAUDE.md. Keep the existing common documentation-discovery
rule unchanged.
In `@reference/STRUCTURE.md`:
- Around line 8-36: Update the Directory Purposes section in STRUCTURE.md so its
component paths are explicitly relative to the project root osac-workspace/,
adding the osac/ prefix to entries such as fulfillment-service/cmd and
osac-operator/cmd. Apply this consistently to all listed component paths while
preserving the existing directory descriptions.
In `@skills/create-pr/SKILL.md`:
- Line 146: Update the `--fix` documentation in the image-tag synchronization
guidance to explicitly exclude `osac-csi-driver` and require manually verifying
and updating its `csiDriver` and `csiBackends` tags, or add an equivalent
CSI-specific check/reminder. Preserve the automatic synchronization guidance for
supported components and state that manual updates remain required until
`sync-image-tags.sh` supports the CSI driver.
- Around line 40-43: Update the repository-name detection in the workflow to
derive the name from Git metadata, preferably remote.origin.url, rather than the
worktree directory basename. Ensure the osac mono-repository detection and
component validation still run for worktrees such as osac-feature-branch,
preserving support for branches that touch multiple component directories.
🪄 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: 95c15edb-c645-4114-8a92-b0958ed29bb1
📒 Files selected for processing (9)
.claude/rules/cross-repo-workflow.md.gitignore.skillsaw.yamlAGENTS.mdREADME.mdbootstrap.shreference/CONVENTIONS.mdreference/STRUCTURE.mdskills/create-pr/SKILL.md
CodeRabbit review, round 1 (4 actionable comments), all fixed: - AGENTS.md: point osac-csi-driver's AGENTS.md column to its README.md as a fallback, matching cross-repo-workflow.md's routing table, which already had this caveat for the same doc-less-component gap. - reference/STRUCTURE.md: add a scoping note clarifying that component-relative paths in Directory Purposes (e.g. fulfillment-service/cmd) are relative to osac/ -- resolves the ambiguity without a large, error-prone rename of the ~35 headers and dozens of body-text path mentions throughout the file that predate this PR. - skills/create-pr/SKILL.md: fix REPO_NAME=$(basename "$REPO_DIR"), which silently breaks mono-repo component detection for any worktree-based change (worktree dirs are named e.g. ../osac-feature-branch per cross-repo-workflow.md, not "osac"). Derive from remote.origin.url instead -- verified correct against the real osac clone from both its main checkout and a real worktree. Pre-existing bug, broader than osac-csi-driver, but small/safe/high-impact enough to fix here rather than defer. - skills/create-pr/SKILL.md: add a manual-verification reminder for osac-csi-driver's csiDriver/csiBackends image tags next to the sync-image-tags.sh guidance, since that script doesn't cover them yet (a gap this PR already documents elsewhere). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
…r-cutover Resolves a conflict in skills/create-pr/SKILL.md between this branch's osac-csi-driver additions and upstream's new tools/resolve-remotes.sh dynamic remote detection (PR osac-project#147). Also updates the REPO_NAME derivation added during /respond to use $UPSTREAM_REMOTE (resolved by resolve-remotes.sh) instead of a hardcoded remote.origin.url guess, since the whole point of resolve-remotes.sh is to not assume which remote name is upstream. Signed-off-by: Tommy Hughes <tohughes@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.claude/rules/cross-repo-workflow.md (1)
61-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a blank line after the heading.
markdownlint-cli2reports MD022 because### Pushing and PR Submissionis immediately followed by a list item. Add one blank line after the heading.Proposed fix
### Pushing and PR Submission + - **Always push to `$PUSH_REMOTE`**, never to `$UPSTREAM_REMOTE`🤖 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 @.claude/rules/cross-repo-workflow.md at line 61, Add one blank line immediately after the “Pushing and PR Submission” heading before the following list item to satisfy Markdown heading-spacing requirements.Source: Linters/SAST tools
bootstrap.sh (2)
183-195: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAvoid returning
originwhen the expected upstream URL is missing.
find_upstream_remotereturnsoriginwhen no remote URL matches${GITHUB_ORG}/${repo}, and callers use that value forgit fetch "$upstream_remote"/git rebase "$upstream_remote/main"or asupstream_remoteinremote get-url $upstream_remote. A clone can haveoriginpoint to a fork or unrelated repo, ororigincan be missing entirely. Return failure when no matching URL is found, and stop or request confirmation in the callers.🤖 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 `@bootstrap.sh` around lines 183 - 195, Update find_upstream_remote so it returns a failure status instead of echoing origin when no remote URL matches expected_suffix. Modify every caller that uses upstream_remote for git fetch, git rebase, or remote get-url to detect this failure and stop safely or explicitly request confirmation before continuing.Source: Coding guidelines
252-256: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate the fork URL with an exact repository match.
The condition at Line 254 accepts any URL that contains
${GH_USER}/${repo}. Forrepo=osac,${GH_USER}/osac-csi-drivermatches and can be kept as the push remote. The condition also ignoresFORK_OVERRIDES[$repo], althoughensure_fork_remoteuses it at Line 90. Bootstrap can push commits to an unintended repository.Compare the normalized URL suffix with
${GH_USER}/${FORK_OVERRIDES[$repo]:-$repo}. Support both/and:URL separators.Proposed fix
+fork_url_matches() { + local url="${1%.git}" expected="$2" + [[ "$url" == *"/${expected}" || "$url" == *":${expected}" ]] +} + + expected_fork_repo="${FORK_OVERRIDES[$repo]:-$repo}" existing_url=$(git -C "$dir" remote get-url "$FORK_REMOTE_NAME" 2>/dev/null) || existing_url="" - if [ -z "$existing_url" ] || [[ "${existing_url%.git}" != *"${GH_USER}/${repo}"* ]]; then + if [ -z "$existing_url" ] || + ! fork_url_matches "$existing_url" "${GH_USER}/${expected_fork_repo}"; thenAs per coding guidelines, the push target must be the developer fork and remote roles must be resolved from URLs.
🤖 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 `@bootstrap.sh` around lines 252 - 256, Update the fork URL validation in the NO_FORK block to compare the normalized remote URL against the exact developer repository suffix `${GH_USER}/${FORK_OVERRIDES[$repo]:-$repo}`, rather than using a substring match. Normalize both URL forms so repository paths using either `/` or `:` separators are accepted, while preserving the existing behavior of calling ensure_fork_remote when the remote is missing or mismatched.Source: Coding guidelines
🤖 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 @.claude/rules/cross-repo-workflow.md:
- Line 61: Add one blank line immediately after the “Pushing and PR Submission”
heading before the following list item to satisfy Markdown heading-spacing
requirements.
In `@bootstrap.sh`:
- Around line 183-195: Update find_upstream_remote so it returns a failure
status instead of echoing origin when no remote URL matches expected_suffix.
Modify every caller that uses upstream_remote for git fetch, git rebase, or
remote get-url to detect this failure and stop safely or explicitly request
confirmation before continuing.
- Around line 252-256: Update the fork URL validation in the NO_FORK block to
compare the normalized remote URL against the exact developer repository suffix
`${GH_USER}/${FORK_OVERRIDES[$repo]:-$repo}`, rather than using a substring
match. Normalize both URL forms so repository paths using either `/` or `:`
separators are accepted, while preserving the existing behavior of calling
ensure_fork_remote when the remote is missing or mismatched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dde03512-aabf-4360-a5fe-84d8f2e43bcc
📒 Files selected for processing (5)
.claude/rules/cross-repo-workflow.mdAGENTS.mdREADME.mdbootstrap.shskills/create-pr/SKILL.md
5 tasks
Contributor
Author
|
Thanks for the follow-up review on the merge commit — all 3 "outside diff range" findings here are legitimate (verified each against the actual code), but they're in
See #185 for the fixes, verification, and test plan. |
4 tasks
openshift-merge-bot Bot
pushed a commit
that referenced
this pull request
Aug 5, 2026
CodeRabbit's review of PR #183's merge commit (which pulled in tools/resolve-remotes.sh from #147) flagged three issues in code that PR #147 introduced. PR #183 merged before these could be addressed, so fixing them here instead: - find_upstream_remote() silently returned "origin" when no remote matched osac-project/<repo>, which could be wrong or missing. Now returns failure instead; all three call sites handle it explicitly (two were already unreachable in practice since is_expected_clone guards them with the same match logic, but are now guarded defensively; the third, the "Available repos" summary, was genuinely unguarded and now shows "unknown" instead of guessing). - The existing-fork-remote check used an unanchored substring match (*"$GH_USER/$repo"*), so a fork URL for a longer repo name sharing a prefix (e.g. osac-csi-driver) would false-positive as a match for "osac". It also ignored FORK_OVERRIDES, unlike ensure_fork_remote's own use of it. Fixed to an end-anchored, FORK_OVERRIDES-aware match, consistent with is_expected_clone's existing pattern. - Added a missing blank line after three "###" headings in cross-repo-workflow.md for markdownlint MD022 (one flagged directly, two more with the identical pattern fixed for consistency). Verified: live bootstrap.sh runs in both --no-fork and default (fork) modes, isolated bash tests reproducing both the substring-match and FORK_OVERRIDES bugs against the old vs. new logic, shellcheck, and make skillsaw. Signed-off-by: Tommy Hughes <tohughes@redhat.com>
openshift-merge-bot Bot
added a commit
that referenced
this pull request
Aug 5, 2026
…n-bugs NO-ISSUE: Fix bootstrap.sh remote-detection bugs found in PR #183 review
openshift-merge-bot Bot
pushed a commit
that referenced
this pull request
Aug 5, 2026
Re-checked osac-project/osac's commit history for drift since the csi-driver cutover (PR #183/#185). Two real components were found migrated/removed further (osac-metering created in-repo, kind-dev migrated from this workspace) -- osac-metering is intentionally not onboarded here since it was never a standalone repo to migrate, but the following stale references were fixed: - scripts/sync-image-tags.sh was deleted upstream (OSAC-3367, which dropped image-tag pinning entirely for mono-repo-resident components -- values/*/values.yaml now use unpinned `latest`, and real release tags are set automatically by osac's own CI). Removed now-false guidance to run it from AGENTS.md, reference/CONVENTIONS.md, and skills/create-pr/SKILL.md. - Leftover `git submodule update --init --recursive` calls in skills/osac-cluster/SKILL.md, skills/osac-cluster/references/refresh.md, and kind-dev/setup.sh -- no .gitmodules exists anywhere in osac anymore (confirmed against current origin/main), so these were dead no-ops left over from the pre-mono-repo layout. - Documented (AGENTS.md) that osac now ships its own root AGENTS.md/tools/bootstrap.sh (OSAC-3557) for standalone use outside this workspace -- warns against running it from within osac-workspace, which would install a second, out-of-sync .ai-workflows clone alongside this workspace's own. Confirmed osac-workspace/kind-dev/ and osac/kind-dev/ are NOT duplicates despite the migration: each is adapted for its own directory depth (osac-workspace's copy reaches through `osac/`, the migrated copy reaches its mono-repo siblings directly) -- left both in place. Verified: skillsaw lint (full repo + osac-cluster/create-pr scoped runs) A grade, 0 errors/warnings; bash -n and shellcheck on the modified shell scripts; full-repo grep confirms no remaining live-doc references to sync-image-tags.sh or git submodule (only historical .artifacts/, quarantined .legacy-repos/, and osac's own files still mention them, which is expected). Part of OSAC-1739. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OSAC-1739: Extend mono-repo tooling to osac-csi-driver cutover
Jira: https://redhat.atlassian.net/browse/OSAC-1739
Summary
Fourth increment under OSAC-1739.
osac-csi-driverhas since cut over from a git submodule to a fullosac-project/osacsubdirectory (OSAC-3530), joiningfulfillment-service/osac-operator/osac-aapfrom increment 1 (#179) andosac-installer/bare-metal-fulfillment-operatorfrom increment 2 (#181). Increment 3 (#182) was an unrelated post-merge documentation audit. This PR extends the same established treatment toosac-csi-driver— everyosac-workspacedoc/rule/skill/script now treats all 6 mono-repo components uniformly asosac/<component>/subdirectories.Changes
bootstrap.sh— removedosac-csi-driverfromREPOS, added it toMERGED_COMPONENTSso it's quarantined to.legacy-repos/instead of cloned independently. Verified live, end-to-end, against this machine's real standaloneosac-csi-driverclone: quarantine preserves the untracked working tree, prints the expected banner, is idempotent on re-run, and doesn't disturb the 5 already-quarantined components.AGENTS.md,README.md) — addedosac-csi-driver's previously-missing Repository Structure and Build/Test rows, extended the merge-history note and Cross-Component Changes list, and rewrote the Deployment Coordination bullet to drop the stale "one remaining git submodule" framing (confirmed zero submodules remain inosac) while accurately flagging thatsync-image-tags.shdoesn't cover its image tag yet — a real upstream gap, documented rather than papered over..claude/rules/cross-repo-workflow.md— added itsCLAUDE.mdrouting row (with a "noCLAUDE.mdyet" caveat, since none exists upstream) and bumped "all five" → "all six" component counts..skillsaw.yaml/.gitignore— foldedosac-csi-driverinto the same migration-window exclude/ignore blocks as the other 5 components.skills/create-pr/SKILL.md— extended the mono-repo detection regex and File Classification table, and added anosac-csi-drivervalidation block. A full-branch self-review caught two gaps here, both fixed: the new block was initially missing themake fmt && git diff --exit-codedrift gate every other Go component's block runs first (itsMakefilehasbuild/testsilently depend onfmt), and theosac-installerblock still referencedgit submodule update --init --recursive, which this PR's own "zero submodules remain" claim made stale.reference/STRUCTURE.md/CONVENTIONS.md— hand-corrected (norepo-intelregeneration tool exists) to nestosac-csi-driverunderosac/and fold it into thesync-image-tags.shcoverage row instead of a standalone submodule row. While inSTRUCTURE.md, discovered none of the other 5 mono-repo components were actually nested under anosac/node in the tree diagram either — restructured the full tree for consistency, not justosac-csi-driver's entry.Non-goals (left untouched, tracked elsewhere):
skills/osac-release/(OSAC-3396); fixingsync-image-tags.shor addingAGENTS.md/CLAUDE.mdtoosac/osac-csi-driver/upstream (gaps in theosacrepo, notosac-workspace); archiving the old standaloneosac-csi-driverrepo (OSAC-1737).Testing
make skillsaw(0 errors/warnings, Grade A both before and after fixes), a livebootstrap.sh --no-forkrun against a real standaloneosac-csi-driverclone (quarantine + idempotency + no regression to the other 5 components confirmed),shellcheck(no new findings), and a manual markdown link review. An independent subagent also ran a full-branch self-review against the realosac/osac-csi-driverclone's actual Makefile targets and Chart.yaml — 2 MEDIUM findings (no CRITICAL/HIGH), both fixed and re-verified. Full details:.artifacts/implement/OSAC-1739/05-validation-report.md.Acceptance Criteria
bootstrap.shquarantines staleosac-csi-driverstandalone clones, stops cloning it independentlyAGENTS.md,README.md) documentosac-csi-driverunderosac/, including previously-missing rows, and accurately describe the submodule removal +sync-image-tags.shgap.claude/rules/cross-repo-workflow.mdrouting/counts updated.skillsaw.yamlexcludes match new layout.gitignoregrouping consistent with.skillsaw.yamlcreate-prskill detects/validatesosac-csi-driverosac-csi-driverskills/osac-release/, upstreamosacrepo gaps, archival,osac-ui/osac-test-infra/enhancement-proposals/docs)Summary by CodeRabbit
Documentation
Chores