CLID-638: Add ARCHITECTURE.md#1460
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@dorzel: This pull request references CLID-638 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 story 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. |
WalkthroughThis PR adds a new ARCHITECTURE.md document describing oc-mirror v2's architecture, including workflow modes, execution flow, directory structure, executor lifecycle, collector system, core data structures, batch worker behavior, archive/history mechanism, cache registry, cluster resource generation, delete workflow, and catalog rebuilding. ChangesArchitecture documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dorzel The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 `@ARCHITECTURE.md`:
- Around line 77-82: The `CopyImageSchema` summary in `ARCHITECTURE.md` is
missing the `RebuiltTag` field, so update the contract description to include it
alongside `Source`, `Destination`, `Origin`, and `Type`. Use the
`CopyImageSchema` symbol as the anchor and add a brief note that `RebuiltTag`
identifies rebuilt catalogs in the rebuild flow, keeping the `CollectorSchema`
description unchanged.
- Around line 17-41: The flowchart fence in ARCHITECTURE.md is missing a
language label, which triggers MD040. Update the existing diagram block to use a
fenced language tag such as text (or convert the diagram to Mermaid) so the
intent stays explicit and lint-clean; keep the change localized to the flowchart
section and preserve the current diagram content.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: daf06a28-da28-40a8-be83-52197636df02
📒 Files selected for processing (1)
ARCHITECTURE.md
| ``` | ||
| ImageSetConfiguration (YAML) | ||
| | | ||
| Read config | ||
| | | ||
| Initialize <- determine workflow mode, set up modules | ||
| | | ||
| Start execution <- start local cache registry, dispatch to workflow | ||
| | | ||
| +--- Mirror to Disk / Disk to Mirror / Mirror to Mirror | ||
| | | ||
| Collect all images | ||
| | | | | | ||
| Release Op Add Helm <- each produces []CopyImageSchema | ||
| | | | | | ||
| +----+----+----+ | ||
| | | ||
| Exclude blocked + deduplicate | ||
| | | ||
| Rebuild catalogs <- M2D and M2M only | ||
| | | ||
| Batch worker <- concurrent image copy | ||
| | | ||
| Build archive (M2D) / Cluster resources generation (D2M, M2M) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the flowchart fence.
This block trips MD040 as written. Use text (or convert it to Mermaid) so the diagram stays lint-clean and its intent is explicit.
Suggested fix
-```
+```textAs per the MD040 warning, fenced code blocks need a language tag.
📝 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.
| ``` | |
| ImageSetConfiguration (YAML) | |
| | | |
| Read config | |
| | | |
| Initialize <- determine workflow mode, set up modules | |
| | | |
| Start execution <- start local cache registry, dispatch to workflow | |
| | | |
| +--- Mirror to Disk / Disk to Mirror / Mirror to Mirror | |
| | | |
| Collect all images | |
| | | | | | |
| Release Op Add Helm <- each produces []CopyImageSchema | |
| | | | | | |
| +----+----+----+ | |
| | | |
| Exclude blocked + deduplicate | |
| | | |
| Rebuild catalogs <- M2D and M2M only | |
| | | |
| Batch worker <- concurrent image copy | |
| | | |
| Build archive (M2D) / Cluster resources generation (D2M, M2M) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@ARCHITECTURE.md` around lines 17 - 41, The flowchart fence in ARCHITECTURE.md
is missing a language label, which triggers MD040. Update the existing diagram
block to use a fenced language tag such as text (or convert the diagram to
Mermaid) so the intent stays explicit and lint-clean; keep the change localized
to the flowchart section and preserve the current diagram content.
Source: Linters/SAST tools
| **CopyImageSchema** (`internal/pkg/api/v2alpha1`) -- the central unit of work representing one image to copy: | ||
| - `Source` / `Destination` -- full image references with transport prefix | ||
| - `Origin` -- original image reference for tracking | ||
| - `Type` -- an `ImageType` enum classifying the image (e.g., `TypeOCPRelease`, `TypeOperatorCatalog`, `TypeGeneric`) | ||
|
|
||
| **CollectorSchema** -- aggregated output from all collectors, carrying the total image counts, the full `[]CopyImageSchema` list, and operator-specific mappings. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add RebuiltTag to CopyImageSchema.
The schema summary omits RebuiltTag, which is part of the actual CopyImageSchema contract and is used to identify rebuilt catalogs in the rebuild flow.
Suggested fix
- `Type` -- an `ImageType` enum classifying the image (e.g., `TypeOCPRelease`, `TypeOperatorCatalog`, `TypeGeneric`)
+- `RebuiltTag` -- marks rebuilt catalogs used by the catalog-rebuild flowAs per the provided internal/pkg/api/v2alpha1/type_internal.go snippet, this field exists on the real struct.
📝 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.
| **CopyImageSchema** (`internal/pkg/api/v2alpha1`) -- the central unit of work representing one image to copy: | |
| - `Source` / `Destination` -- full image references with transport prefix | |
| - `Origin` -- original image reference for tracking | |
| - `Type` -- an `ImageType` enum classifying the image (e.g., `TypeOCPRelease`, `TypeOperatorCatalog`, `TypeGeneric`) | |
| **CollectorSchema** -- aggregated output from all collectors, carrying the total image counts, the full `[]CopyImageSchema` list, and operator-specific mappings. | |
| **CopyImageSchema** (`internal/pkg/api/v2alpha1`) -- the central unit of work representing one image to copy: | |
| - `Source` / `Destination` -- full image references with transport prefix | |
| - `Origin` -- original image reference for tracking | |
| - `Type` -- an `ImageType` enum classifying the image (e.g., `TypeOCPRelease`, `TypeOperatorCatalog`, `TypeGeneric`) | |
| - `RebuiltTag` -- marks rebuilt catalogs used by the catalog-rebuild flow | |
| **CollectorSchema** -- aggregated output from all collectors, carrying the total image counts, the full `[]CopyImageSchema` list, and operator-specific mappings. |
🤖 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 `@ARCHITECTURE.md` around lines 77 - 82, The `CopyImageSchema` summary in
`ARCHITECTURE.md` is missing the `RebuiltTag` field, so update the contract
description to include it alongside `Source`, `Destination`, `Origin`, and
`Type`. Use the `CopyImageSchema` symbol as the anchor and add a brief note that
`RebuiltTag` identifies rebuilt catalogs in the rebuild flow, keeping the
`CollectorSchema` description unchanged.
|
@dorzel: all tests passed! 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
Description
Add ARCHITECTURE.md. Tried to keep this high level and not code specific. Let me know if the "High-Level Flow" graph is helpful or not.
Github / Jira issue: https://redhat.atlassian.net/browse/CLID-638
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Expected Outcome
Summary by CodeRabbit