NO-JIRA: docs: fill gaps in CONTRIBUTING.md and ARCHITECTURE.md - #302
NO-JIRA: docs: fill gaps in CONTRIBUTING.md and ARCHITECTURE.md#302jparrill wants to merge 1 commit into
Conversation
CONTRIBUTING.md: - Add PR Process section with full lifecycle overview - Fix CI Pipeline table to match actual Prow jobs (build, unit, verify, security, images) - Document CODEOWNERS vs OWNERS relationship in Review Process - Add Backporting section for release branch cherry-picks ARCHITECTURE.md: - Add Design Overview section explaining kind-based dispatch model - Add pkg/version to Core Components table - Explain ConfigMap discovery via pod SA namespace mount - Add Failure Modes section with recovery guidance - Add mermaid sequence diagrams for backup and restore flows - Add Tradeoffs and Decisions section covering 6 key design choices Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request explicitly references no jira issue. 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. |
WalkthroughDocumentation updates expand the plugin architecture reference and contributor guidance, covering backup/restore flows, configuration, failure modes, design decisions, pull request procedures, CI checks, review ownership, and release-branch backporting. ChangesArchitecture documentation
Contributor documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill 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
![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 `@ARCHITECTURE.md`:
- Line 11: Update the architecture statement describing plugin statelessness to
remove the claim that statelessness alone makes retries safe. State that retry
safety comes from explicit idempotency and resource-creation guards, while
preserving the documented in-memory result-caching behavior referenced
elsewhere.
- Line 120: Update the ShouldEndPluginExecution documentation row to distinguish
the true, nil non-HCP backup path from the true, error path caused by empty
IncludedNamespaces; limit the “returns the item unmodified” and “No action
needed” statements to true, nil, and document the error outcome separately.
- Around line 140-141: Update the architecture diagrams around the
NewBackupPlugin and NewRestorePlugin flows to show ConfigMap loading during
plugin initialization, before the first Execute/item-processing step. Move the
“Get ConfigMap (plugin config)” interaction accordingly or label it explicitly
as initialization, and apply the same correction to both referenced diagram
sections.
In `@CONTRIBUTING.md`:
- Line 93: Update the CONTRIBUTING.md guidance for CODEOWNERS and OWNERS to
state that CODEOWNERS identifies requested owners while GitHub branch-protection
settings enforce required approvals. Reconcile the listed owners by aligning
both files, or explicitly document that differences such as joeavaikath and
devguyio are intentional.
🪄 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: 17ee2b6e-e989-42ef-8c08-f9b164698164
📒 Files selected for processing (2)
ARCHITECTURE.mdCONTRIBUTING.md
|
|
||
| Backup adds metadata (etcd snapshot URLs, restore annotations) and excludes resources that should not be persisted (pods, etcd PVCs when using snapshot method). Restore reads that metadata back, generates time-limited signed URLs for etcd snapshot download, and injects them into the restored resources so HyperShift can bootstrap the control plane from the snapshot. | ||
|
|
||
| The plugin is stateless across invocations — all coordination state flows through Kubernetes resources (annotations, CRs) rather than in-memory caches. This makes it safe for Velero to retry or re-invoke the plugin without side effects. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not equate statelessness with retry safety.
This overstates the guarantee and conflicts with the documented in-memory result caching in line 44. Clarify that supported retries are safe because of explicit idempotency and creation guards, not merely because the plugin is stateless.
🤖 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` at line 11, Update the architecture statement describing
plugin statelessness to remove the claim that statelessness alone makes retries
safe. State that retry safety comes from explicit idempotency and
resource-creation guards, while preserving the documented in-memory
result-caching behavior referenced elsewhere.
| | Failure | What Happens | Recovery | | ||
| |---------|-------------|----------| | ||
| | **ConfigMap missing** | Plugin proceeds with defaults (`volumeSnapshot`, `hypershift` namespace). Not an error. | Create the ConfigMap if non-default behavior is needed. | | ||
| | **`ShouldEndPluginExecution` returns true** | Plugin returns the item unmodified. This is normal for non-HCP backups — no error is raised. | No action needed. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinguish true, nil from true, error.
ShouldEndPluginExecution returns true with an error when IncludedNamespaces is empty. Limit this row’s no-op statement to the non-HCP true, nil path.
🤖 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` at line 120, Update the ShouldEndPluginExecution
documentation row to distinguish the true, nil non-HCP backup path from the
true, error path caused by empty IncludedNamespaces; limit the “returns the item
unmodified” and “No action needed” statements to true, nil, and document the
error outcome separately.
| BIA->>K8s: Get ConfigMap (plugin config) | ||
| BIA->>BIA: Validate platform config |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show ConfigMap loading during plugin initialization.
NewBackupPlugin and NewRestorePlugin read the ConfigMap before Execute handles items. The diagrams currently imply a Kubernetes read for each item; move this step before the first execution or label it as initialization.
Also applies to: 168-169
🤖 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 140 - 141, Update the architecture diagrams
around the NewBackupPlugin and NewRestorePlugin flows to show ConfigMap loading
during plugin initialization, before the first Execute/item-processing step.
Move the “Get ConfigMap (plugin config)” interaction accordingly or label it
explicitly as initialization, and apply the same correction to both referenced
diagram sections.
| ## Review Process | ||
|
|
||
| - Reviewers and approvers are listed in the [OWNERS](./OWNERS) file. Any reviewer can provide feedback; approvers have merge authority. | ||
| - [`.github/CODEOWNERS`](.github/CODEOWNERS) gates GitHub's "required reviews" feature — it controls who GitHub notifies and requires approval from. `OWNERS` controls Prow's `/lgtm` and `/approve` commands. Both files should stay in sync. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Clarify CODEOWNERS enforcement and ownership differences.
CODEOWNERS identifies requested owners; GitHub branch-protection settings enforce required approvals. Also, the supplied files are not currently synchronized: OWNERS includes joeavaikath and devguyio, while .github/CODEOWNERS does not. Either align the lists or document that the difference is intentional.
🤖 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 `@CONTRIBUTING.md` at line 93, Update the CONTRIBUTING.md guidance for
CODEOWNERS and OWNERS to state that CODEOWNERS identifies requested owners while
GitHub branch-protection settings enforce required approvals. Reconcile the
listed owners by aligning both files, or explicitly document that differences
such as joeavaikath and devguyio are intentional.
|
@jparrill: 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. |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Four findings from review — one blocking (the images CI row), three suggestions.
| | `build` | Code compiles successfully | | ||
| | `unit` | All unit and integration tests pass | | ||
| | `verify` | Combined module verification (`go.mod`/`go.sum` up to date) + tests | | ||
| | `security` | Scans for known vulnerabilities in dependencies | |
There was a problem hiding this comment.
nit: Worth noting that this job has skip_if_only_changed: ^docs/|\.md$|... in the Prow config — it won't run on doc-only PRs. A contributor submitting a markdown-only change won't see this check and might think something is broken.
|
|
||
| Backup adds metadata (etcd snapshot URLs, restore annotations) and excludes resources that should not be persisted (pods, etcd PVCs when using snapshot method). Restore reads that metadata back, generates time-limited signed URLs for etcd snapshot download, and injects them into the restored resources so HyperShift can bootstrap the control plane from the snapshot. | ||
|
|
||
| The plugin is stateless across invocations — all coordination state flows through Kubernetes resources (annotations, CRs) rather than in-memory caches. This makes it safe for Velero to retry or re-invoke the plugin without side effects. |
There was a problem hiding this comment.
"The plugin is stateless across invocations — all coordination state flows through Kubernetes resources (annotations, CRs) rather than in-memory caches."
This contradicts the Design Invariants section below, which says: "The etcd orchestrator uses IsCreated() guards and caches results to avoid duplicate work." One claims no in-memory caches; the other documents their existence. Reconcile these — either qualify the stateless claim (e.g., "stateless across Velero invocations but may cache within a single backup run") or update the invariants section.
| ## Review Process | ||
|
|
||
| - Reviewers and approvers are listed in the [OWNERS](./OWNERS) file. Any reviewer can provide feedback; approvers have merge authority. | ||
| - [`.github/CODEOWNERS`](.github/CODEOWNERS) gates GitHub's "required reviews" feature — it controls who GitHub notifies and requires approval from. `OWNERS` controls Prow's `/lgtm` and `/approve` commands. Both files should stay in sync. |
There was a problem hiding this comment.
"Both files should stay in sync" — but they aren't today. OWNERS includes joeavaikath (approver+reviewer) and devguyio (reviewer), neither of whom appear in CODEOWNERS. Either sync them in this PR or soften the language to acknowledge the drift, e.g., "These files serve different systems (Prow vs GitHub) and should be kept consistent where practical."
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Correction: my earlier images comment was wrong — ci/prow/images is a real check auto-generated by ci-operator from the images: stanza. I've deleted that comment. The CI table is accurate.
The remaining 3 inline comments (stateless/cache contradiction, CODEOWNERS sync gap, security skip behavior) are suggestions, not blockers. Withdrawing the changes-requested status.
3 participants
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit