OAPE-881: Added agentic docs - #379
Open
shivprakashmuley wants to merge 2 commits into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@shivprakashmuley: This pull request references OAPE-881 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. |
WalkthroughAdded documentation for repository onboarding, development, testing, architecture, CRD behavior, architectural decisions, execution plans, ecosystem practices, and enhancement proposals. ChangesMust-Gather Operator 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shivprakashmuley 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #379 +/- ##
=======================================
Coverage 82.57% 82.57%
=======================================
Files 9 9
Lines 918 918
=======================================
Hits 758 758
Misses 148 148
Partials 12 12 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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`:
- Line 6: Update the AI agents guidance in AGENTS.md to reference the actual
documentation locations under ai-docs/: ai-docs/domain/, ai-docs/architecture/,
and ai-docs/decisions/. Include ai-docs/MGO_DEVELOPMENT.md as the development
guide, and apply the corrected paths to both documented references.
In `@ai-docs/architecture/components.md`:
- Around line 101-111: Align cleanup documentation in
ai-docs/architecture/components.md:101-111 and
ai-docs/domain/mustgather.md:136-142, and remove the six-hour cleanup claim from
README.md. State that cleanup occurs during the same reconciliation after Job
success or failure when retainResourcesOnCompletion is unset or false, deleting
owned Pods and Jobs and cleaning configured trusted CA ConfigMaps; on CR
deletion, the finalizer performs this cleanup only when retention is disabled.
In `@ai-docs/decisions/adr-0001-immutable-spec.md`:
- Around line 21-26: Update the “Jobs are immutable once created” rationale in
ADR-0001 to avoid claiming absolute Kubernetes Job immutability; state instead
that gather execution inputs cannot be changed safely in place, while preserving
the surrounding one-shot semantics and controller simplification rationale.
In `@ai-docs/decisions/adr-0002-two-container-job.md`:
- Around line 11-18: Update the Decision section to state that the Job always
includes the gather container, while the upload container is added only when
uploadTarget is configured. Clarify that the two-container design and
cross-container polling apply to upload-enabled jobs, while gather-only jobs run
without the upload container.
- Around line 27-32: Update the “Upload container failure doesn't require
re-running the gather” consequence in the Consequences section to state that
upload failures can cause a new Pod and rerun the gather container under
restartPolicy Never and backoffLimit 3. Only retain the existing claim if the
documented design explicitly includes retry logic that skips gathering.
In `@ai-docs/MGO_DEVELOPMENT.md`:
- Around line 52-58: Update stale CRD filename references to
deploy/crds/operator.openshift.io_mustgathers.yaml in README.md and CLAUDE.md.
The reference shown in ai-docs/MGO_DEVELOPMENT.md lines 52-58 already uses the
correct filename and requires no change; inspect
ai-docs/architecture/components.md lines 241-248 and update its reference if
stale.
In `@ai-docs/MGO_TESTING.md`:
- Around line 74-83: Update the Go test-table example so the “With PVC” entry
uses valid mustgatherv1alpha1.Storage composite-literal fields instead of
`{...}`; if concrete fields cannot be shown, explicitly mark the block as
pseudocode. Keep the other test cases unchanged.
- Around line 115-117: Update the specific-test command in MGO_TESTING.md to use
the Ginkgo focus flag, replacing the Go test -run filter with
-ginkgo.focus="test description" while preserving the existing test options.
In `@ai-docs/references/ecosystem.md`:
- Line 7: Correct the external documentation references across
ai-docs/references/ecosystem.md (lines 7, 22, 34, 46, 56, 67, 78, and 84) and
ai-docs/exec-plans/README.md (line 3): link the existing operator-patterns,
Kubernetes, and OpenShift entries to their specific directories instead of the
ai-docs root, mark missing testing, security, reliability, decisions, and
Exec-Plans references as planned or link them to published documents, and
clarify the accompanying note.
In `@ai-docs/references/enhancements.md`:
- Around line 26-29: Update the documentation index section around the “No
dedicated design directories” text to link `ai-docs/domain/`,
`ai-docs/architecture/`, and `ai-docs/decisions/` in that order, then retain
`CLAUDE.md` and `README.md` as supplemental references. Remove the outdated
claim that dedicated design directories do not exist.
In `@README.md`:
- Around line 171-186: Resolve the broken links in the README.md “Development
Guidelines” section by either adding the six referenced guideline files under
docs/ with valid content or removing their corresponding list entries; ensure no
links point to missing targets.
🪄 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: 4dc472ef-7f4a-4892-9f5c-ce71b5f46bea
📒 Files selected for processing (13)
AGENTS.mdREADME.mdai-docs/MGO_DEVELOPMENT.mdai-docs/MGO_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-immutable-spec.mdai-docs/decisions/adr-0002-two-container-job.mdai-docs/decisions/adr-0003-extensible-upload-union.mdai-docs/decisions/adr-template.mdai-docs/domain/mustgather.mdai-docs/exec-plans/README.mdai-docs/references/ecosystem.mdai-docs/references/enhancements.md
Comment on lines
+101
to
+111
| ### Cleanup (Event-Driven, NOT Timer-Based) | ||
|
|
||
| `cleanupMustGatherResources()` runs on **job completion** (success or failure) and **CR deletion** (via finalizer). There is no scheduled garbage collection timer. | ||
|
|
||
| Cleanup steps: | ||
| 1. Verify Job has ownerReference matching MustGather UID (skip if not owned) | ||
| 2. List and delete pods with label `controller-uid=<Job.UID>` | ||
| 3. Delete the Job | ||
| 4. Remove trusted CA ConfigMap ownerReference (delete ConfigMap if last owner) | ||
|
|
||
| Skipped entirely when `retainResourcesOnCompletion: true`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(README\.md|components\.md|mustgather\.md)$|must.?gather|MustGather' | head -200
printf '%s\n' '--- relevant documentation ---'
for f in ai-docs/architecture/components.md ai-docs/domain/mustgather.md README.md; do
if [ -f "$f" ]; then
echo "### $f"
rg -n -i -C 5 'cleanup|retainResourcesOnCompletion|six hours|6 hours|timer|finalizer|completed' "$f" || true
fi
done
printf '%s\n' '--- controller symbols and cleanup references ---'
rg -n -i -S 'cleanupMustGatherResources|retainResourcesOnCompletion|MustGather|trusted CA|controller-uid' \
--glob '!ai-docs/**' --glob '!README.md' --glob '!**/node_modules/**' . | head -400Repository: openshift/must-gather-operator
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository files ---'
git ls-files | grep -E '(^|/)(README\.md|components\.md|mustgather\.md)$|must.?gather|MustGather' | head -200
printf '%s\n' '--- documentation references ---'
for f in ai-docs/architecture/components.md ai-docs/domain/mustgather.md README.md; do
if [ -f "$f" ]; then
echo "### $f"
rg -n -i -C 5 'cleanup|retainResourcesOnCompletion|six hours|6 hours|timer|finalizer|completed' "$f" || true
fi
done
printf '%s\n' '--- implementation references ---'
rg -n -i -S 'cleanupMustGatherResources|retainResourcesOnCompletion|trusted CA|controller-uid|MustGather' \
--glob '!ai-docs/**' --glob '!README.md' --glob '!**/node_modules/**' . | head -500Repository: openshift/must-gather-operator
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Reconcile and completion paths ---'
sed -n '90,335p' controllers/mustgather/mustgather_controller.go
printf '%s\n' '--- cleanup implementation ---'
sed -n '445,535p' controllers/mustgather/mustgather_controller.go
printf '%s\n' '--- predicates and watches ---'
sed -n '1,180p' controllers/mustgather/predicates.go
sed -n '365,385p' controllers/mustgather/mustgather_controller.go
printf '%s\n' '--- focused cleanup and retention tests ---'
sed -n '420,490p' test/e2e/must_gather_operator_test.go
sed -n '640,885p' test/e2e/must_gather_operator_test.go
printf '%s\n' '--- unit-test cleanup references ---'
rg -n -C 8 'cleanupMustGatherResources|handleJobCompletion|retainResourcesOnCompletion|Reconcile.*DeletionTimestamp' \
controllers/mustgather/*_test.goRepository: openshift/must-gather-operator
Length of output: 34295
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Reconcile and completion paths ---'
sed -n '90,335p' controllers/mustgather/mustgather_controller.go
printf '%s\n' '--- cleanup implementation ---'
sed -n '445,535p' controllers/mustgather/mustgather_controller.go
printf '%s\n' '--- predicates and watches ---'
sed -n '1,180p' controllers/mustgather/predicates.go
sed -n '365,385p' controllers/mustgather/mustgather_controller.go
printf '%s\n' '--- focused cleanup and retention tests ---'
sed -n '420,490p' test/e2e/must_gather_operator_test.go
sed -n '640,885p' test/e2e/must_gather_operator_test.go
printf '%s\n' '--- unit-test cleanup references ---'
rg -n -C 8 'cleanupMustGatherResources|handleJobCompletion|retainResourcesOnCompletion|DeletionTimestamp' controllers/mustgather/*_test.goRepository: openshift/must-gather-operator
Length of output: 39685
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- controller lifecycle ---'
sed -n '99,330p' controllers/mustgather/mustgather_controller.go
printf '%s\n' '--- cleanup function ---'
sed -n '454,532p' controllers/mustgather/mustgather_controller.go
printf '%s\n' '--- watch setup ---'
sed -n '365,385p' controllers/mustgather/mustgather_controller.go
sed -n '1,150p' controllers/mustgather/predicates.go
printf '%s\n' '--- focused tests ---'
sed -n '424,478p' test/e2e/must_gather_operator_test.go
sed -n '656,875p' test/e2e/must_gather_operator_test.go
rg -n -C 5 'cleanupMustGatherResources|handleJobCompletion|retainResourcesOnCompletion|DeletionTimestamp' controllers/mustgather/*_test.goRepository: openshift/must-gather-operator
Length of output: 33113
Align all cleanup documentation with the controller.
Remove the six-hour claim from README.md. State in both cited sections that cleanup runs in the same reconciliation after Job success or failure when retainResourcesOnCompletion is unset or false. The controller deletes owned Pods and Jobs and cleans configured trusted CA ConfigMaps. On CR deletion, the finalizer performs this cleanup only when retention is disabled.
📍 Affects 2 files
ai-docs/architecture/components.md#L101-L111(this comment)ai-docs/domain/mustgather.md#L136-L142
🤖 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 `@ai-docs/architecture/components.md` around lines 101 - 111, Align cleanup
documentation in ai-docs/architecture/components.md:101-111 and
ai-docs/domain/mustgather.md:136-142, and remove the six-hour cleanup claim from
README.md. State that cleanup occurs during the same reconciliation after Job
success or failure when retainResourcesOnCompletion is unset or false, deleting
owned Pods and Jobs and cleaning configured trusted CA ConfigMaps; on CR
deletion, the finalizer performs this cleanup only when retention is disabled.
|
|
||
| ## Operator Patterns | ||
|
|
||
| **Location**: [openshift/enhancements/ai-docs/platform/operator-patterns/](https://github.com/openshift/enhancements/tree/master/ai-docs/) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
printf '%s\n' '--- local references ---'
for f in ai-docs/references/ecosystem.md ai-docs/exec-plans/README.md; do
echo "### $f"
sed -n '1,110p' "$f"
done
printf '%s\n' '--- remote target status ---'
for path in \
ai-docs \
ai-docs/platform/operator-patterns \
ai-docs/practices/testing \
ai-docs/practices/security \
ai-docs/practices/reliability \
ai-docs/domain/kubernetes \
ai-docs/domain/openshift \
ai-docs/decisions; do
url="https://github.com/openshift/enhancements/tree/master/$path"
code=$(curl -L -sS -o /dev/null -w '%{http_code}' "$url")
printf '%-55s %s\n' "$path" "$code"
done
printf '%s\n' '--- remote repository paths ---'
curl -sS 'https://api.github.com/repos/openshift/enhancements/git/trees/master?recursive=1' |
jq -r '.tree[]?.path' |
grep -E '^(ai-docs|docs/|enhancements/)' |
grep -E '(operator-patterns|exec-plan|testing|security|reliability|domain|decisions|ai-docs)' |
head -200Repository: openshift/must-gather-operator
Length of output: 7053
🏁 Script executed:
# Check each exact Markdown URL for redirects and the final response URL.
python3 - <<'PY'
import urllib.request
urls = [
"https://github.com/openshift/enhancements/tree/master/ai-docs/",
"https://github.com/openshift/enhancements/tree/master/ai-docs/platform/operator-patterns/",
"https://github.com/openshift/enhancements/tree/master/ai-docs/practices/testing/",
"https://github.com/openshift/enhancements/tree/master/ai-docs/practices/security/",
"https://github.com/openshift/enhancements/tree/master/ai-docs/practices/reliability/",
"https://github.com/openshift/enhancements/tree/master/ai-docs/domain/kubernetes/",
"https://github.com/openshift/enhancements/tree/master/ai-docs/domain/openshift/",
"https://github.com/openshift/enhancements/tree/master/ai-docs/decisions/",
]
for url in urls:
try:
req = urllib.request.Request(url, method="GET", headers={"User-Agent": "review-check"})
with urllib.request.urlopen(req, timeout=20) as r:
print(r.status, r.geturl(), url)
except Exception as e:
print(type(e).__name__, str(e), url)
PYRepository: openshift/must-gather-operator
Length of output: 1955
Fix the external documentation targets.
The Location labels name specific paths, but every link points to /ai-docs/. Link the existing operator-patterns, Kubernetes, and OpenShift directories directly. Mark the missing testing, security, reliability, decisions, and Exec-Plans references as planned or link to published documents. Clarify the note accordingly.
📍 Affects 2 files
ai-docs/references/ecosystem.md#L7-L7(this comment)ai-docs/exec-plans/README.md#L3-L3ai-docs/references/ecosystem.md#L22-L22ai-docs/references/ecosystem.md#L34-L34ai-docs/references/ecosystem.md#L46-L46ai-docs/references/ecosystem.md#L56-L56ai-docs/references/ecosystem.md#L67-L67ai-docs/references/ecosystem.md#L78-L78ai-docs/references/ecosystem.md#L84-L84
🤖 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 `@ai-docs/references/ecosystem.md` at line 7, Correct the external
documentation references across ai-docs/references/ecosystem.md (lines 7, 22,
34, 46, 56, 67, 78, and 84) and ai-docs/exec-plans/README.md (line 3): link the
existing operator-patterns, Kubernetes, and OpenShift entries to their specific
directories instead of the ai-docs root, mark missing testing, security,
reliability, decisions, and Exec-Plans references as planned or link them to
published documents, and clarify the accompanying note.
Comment on lines
+26
to
+29
| No dedicated design directories exist in this repo. Architecture context is in: | ||
|
|
||
| - `CLAUDE.md` — AI-oriented architecture reference | ||
| - `README.md` — Operator purpose, CR format, deployment instructions |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Index the local design documents added by this PR.
This section says that no dedicated design directories exist, but the repository now contains ai-docs/domain/, ai-docs/architecture/, and ai-docs/decisions/. The current text hides the new architecture documents and ADRs from readers and agents.
Replace this section with links to those directories. Keep CLAUDE.md and README.md as supplemental references.
Based on learnings, architectural work must expose domain documentation, architecture documentation, and applicable decision records in that order.
Proposed index update
## Local Design Documents
-No dedicated design directories exist in this repo. Architecture context is in:
+Repository design documentation is organized as:
+- `ai-docs/domain/`
+- `ai-docs/architecture/`
+- `ai-docs/decisions/`
- `CLAUDE.md` — AI-oriented architecture reference
- `README.md` — Operator purpose, CR format, deployment instructions🤖 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 `@ai-docs/references/enhancements.md` around lines 26 - 29, Update the
documentation index section around the “No dedicated design directories” text to
link `ai-docs/domain/`, `ai-docs/architecture/`, and `ai-docs/decisions/` in
that order, then retain `CLAUDE.md` and `README.md` as supplemental references.
Remove the outdated claim that dedicated design directories do not exist.
Source: Learnings
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ai-docs/architecture/components.md (1)
144-148: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAuthenticate the SFTP server before uploading.
build/bin/uploaddisables host-key verification, and the Job only creates an emptyknown_hostsfile. The pre-flight validation also usesssh.InsecureIgnoreHostKey(). Pin a trusted host key in both paths, or document and track an approved security exception.🤖 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 `@ai-docs/architecture/components.md` around lines 144 - 148, Update the upload workflow and its pre-flight validation to authenticate the SFTP server with a trusted, pinned host key instead of disabling host-key verification. Replace the empty known_hosts setup and ssh.InsecureIgnoreHostKey() usage with the approved host-key configuration, or document and track an explicit security exception if pinning cannot be implemented.
🤖 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`:
- Line 51: Update the AI Agent Path documentation in AGENTS.md to include
ai-docs/MGO_TESTING.md after ai-docs/decisions/ and before or alongside
ai-docs/MGO_DEVELOPMENT.md, making clear that agents must read the testing guide
for test-related changes while retaining the development guide for other
relevant changes.
- Around line 6-8: Remove the blank line between the two consecutive blockquote
lines in AGENTS.md so the AI agents guidance and Platform Patterns guidance
remain contiguous and comply with markdownlint MD028.
In `@ai-docs/references/ecosystem.md`:
- Line 19: Update the ecosystem reference’s Location entry to remove the stale
“planned — directory not yet published” marker, while preserving the existing
operator-patterns URL and link formatting.
In `@CLAUDE.md`:
- Around line 76-79: Update the Job Template and reconciliation flow sections in
CLAUDE.md to consistently describe a mandatory gather container and an upload
container added only when uploadTarget is configured. Revise cleanup wording to
state that cleanup occurs during the current reconciliation after completion,
and on CR deletion only when retainResourcesOnCompletion is unset or false.
In `@README.md`:
- Line 46: Update the README retention statement near “On CR deletion” so it
does not claim that retainResourcesOnCompletion preserves resources after
MustGather CR deletion. Limit the claim to completed Job resources while the CR
exists, or explicitly document that owned resources may be garbage-collected
after the CR and its finalizer are removed.
---
Outside diff comments:
In `@ai-docs/architecture/components.md`:
- Around line 144-148: Update the upload workflow and its pre-flight validation
to authenticate the SFTP server with a trusted, pinned host key instead of
disabling host-key verification. Replace the empty known_hosts setup and
ssh.InsecureIgnoreHostKey() usage with the approved host-key configuration, or
document and track an explicit security exception if pinning cannot be
implemented.
🪄 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: 60a73b7b-8e2f-4691-97e6-6f1cbf8a2df9
📒 Files selected for processing (9)
AGENTS.mdCLAUDE.mdREADME.mdai-docs/MGO_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-immutable-spec.mdai-docs/decisions/adr-0002-two-container-job.mdai-docs/domain/mustgather.mdai-docs/references/ecosystem.md
🚧 Files skipped from review as they are similar to previous changes (3)
- ai-docs/domain/mustgather.md
- ai-docs/decisions/adr-0001-immutable-spec.md
- ai-docs/decisions/adr-0002-two-container-job.md
Comment on lines
+6
to
+8
| > **AI agents**: Read `ai-docs/domain/` first for API types, then `ai-docs/architecture/` for implementation patterns. Check `ai-docs/decisions/` before making architectural changes. | ||
|
|
||
| > **Platform Patterns**: See [openshift/enhancements/ai-docs/](https://github.com/openshift/enhancements/tree/master/ai-docs/) for operator patterns, testing, security, and cross-repo ADRs. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the blank line inside the blockquote.
Line 7 separates two consecutive blockquote lines. This triggers markdownlint rule MD028. Keep the blockquote lines contiguous.
Proposed fix
> **AI agents**: Read `ai-docs/domain/` first for API types, then `ai-docs/architecture/` for implementation patterns. Check `ai-docs/decisions/` before making architectural changes.
-
> **Platform Patterns**: See [openshift/enhancements/ai-docs/](https://github.com/openshift/enhancements/tree/master/ai-docs/) for operator patterns, testing, security, and cross-repo ADRs.📝 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.
Suggested change
| > **AI agents**: Read `ai-docs/domain/` first for API types, then `ai-docs/architecture/` for implementation patterns. Check `ai-docs/decisions/` before making architectural changes. | |
| > **Platform Patterns**: See [openshift/enhancements/ai-docs/](https://github.com/openshift/enhancements/tree/master/ai-docs/) for operator patterns, testing, security, and cross-repo ADRs. | |
| > **AI agents**: Read `ai-docs/domain/` first for API types, then `ai-docs/architecture/` for implementation patterns. Check `ai-docs/decisions/` before making architectural changes. | |
| > **Platform Patterns**: See [openshift/enhancements/ai-docs/](https://github.com/openshift/enhancements/tree/master/ai-docs/) for operator patterns, testing, security, and cross-repo ADRs. |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 7-7: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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 `@AGENTS.md` around lines 6 - 8, Remove the blank line between the two
consecutive blockquote lines in AGENTS.md so the AI agents guidance and Platform
Patterns guidance remain contiguous and comply with markdownlint MD028.
Source: Linters/SAST tools
| └── MGO_TESTING.md # Unit (fake client + interceptClient), E2E (Ginkgo) | ||
| ``` | ||
|
|
||
| **AI Agent Path**: `ai-docs/domain/` → `ai-docs/architecture/` → `ai-docs/decisions/` → `ai-docs/MGO_DEVELOPMENT.md` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include the testing guide in the agent documentation path.
When a change affects tests, agents must read ai-docs/MGO_TESTING.md after ai-docs/decisions/. The current path lists only ai-docs/MGO_DEVELOPMENT.md.
As per coding guidelines: read ai-docs/MGO_DEVELOPMENT.md or ai-docs/MGO_TESTING.md as relevant.
🤖 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 `@AGENTS.md` at line 51, Update the AI Agent Path documentation in AGENTS.md to
include ai-docs/MGO_TESTING.md after ai-docs/decisions/ and before or alongside
ai-docs/MGO_DEVELOPMENT.md, making clear that agents must read the testing guide
for test-related changes while retaining the development guide for other
relevant changes.
Source: Coding guidelines
|
|
||
| ## Operator Patterns | ||
|
|
||
| **Location**: [openshift/enhancements/ai-docs/platform/operator-patterns/](https://github.com/openshift/enhancements/tree/master/ai-docs/platform/operator-patterns/) *(planned — directory not yet published)* |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale planned marker.
The operator-patterns directory is published and contains documentation. The current marker says that the directory is not published, so readers may skip a valid reference. Remove the marker. (github.com)
Proposed fix
-**Location**: [openshift/enhancements/ai-docs/platform/operator-patterns/](https://github.com/openshift/enhancements/tree/master/ai-docs/platform/operator-patterns/) *(planned — directory not yet published)*
+**Location**: [openshift/enhancements/ai-docs/platform/operator-patterns/](https://github.com/openshift/enhancements/tree/master/ai-docs/platform/operator-patterns/)🤖 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 `@ai-docs/references/ecosystem.md` at line 19, Update the ecosystem reference’s
Location entry to remove the stale “planned — directory not yet published”
marker, while preserving the existing operator-patterns URL and link formatting.
Comment on lines
+76
to
+79
| - Creates Kubernetes Jobs with a gather container and a conditional upload container (added only when uploadTarget is configured) | ||
| - Handles finalizers for proper cleanup of jobs, pods, and trusted CA ConfigMaps | ||
| - Cleanup runs in the same reconciliation after Job success or failure when `retainResourcesOnCompletion` is unset or false | ||
| - On CR deletion, the finalizer performs this cleanup only when retention is disabled |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the conditional Job model consistent throughout CLAUDE.md.
Line 76 states that the upload container is added only when uploadTarget is configured. The Job Template section at Lines 82-85 still describes every Job as having two containers. The reconciliation flow at Lines 99-109 still describes unconditional cleanup and delayed deletion.
Update those sections to state that the gather container is always present, the upload container is conditional, and cleanup runs during the current reconciliation when retention is disabled.
🤖 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.md` around lines 76 - 79, Update the Job Template and reconciliation
flow sections in CLAUDE.md to consistently describe a mandatory gather container
and an upload container added only when uploadTarget is configured. Revise
cleanup wording to state that cleanup occurs during the current reconciliation
after completion, and on CR deletion only when retainResourcesOnCompletion is
unset or false.
|
|
||
| When a MustGather Job succeeds or fails, the controller runs cleanup in the same reconciliation pass: it deletes owned Pods and Jobs and removes any configured trusted CA ConfigMaps. This behavior applies when `retainResourcesOnCompletion` is unset or `false`. | ||
|
|
||
| On CR deletion, the finalizer performs this same cleanup only when retention is disabled. Setting `retainResourcesOnCompletion: true` preserves all Job resources for inspection. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not claim that retention preserves resources after CR deletion.
retainResourcesOnCompletion: true skips the controller's explicit cleanup, but the finalizer is then removed and the MustGather CR is deleted. The controller notes that owned resources can be garbage-collected after the CR disappears.
Limit the retention claim to completed Jobs while the MustGather CR still exists, or document the owner-reference garbage-collection behavior.
Proposed wording
-On CR deletion, the finalizer performs this same cleanup only when retention is disabled. Setting `retainResourcesOnCompletion: true` preserves all Job resources for inspection.
+On CR deletion, the finalizer performs this same cleanup only when retention is disabled. Setting `retainResourcesOnCompletion: true` skips explicit cleanup while the `MustGather` CR exists; deleting the CR can still trigger Kubernetes garbage collection of owned resources.📝 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.
Suggested change
| On CR deletion, the finalizer performs this same cleanup only when retention is disabled. Setting `retainResourcesOnCompletion: true` preserves all Job resources for inspection. | |
| On CR deletion, the finalizer performs this same cleanup only when retention is disabled. Setting `retainResourcesOnCompletion: true` skips explicit cleanup while the `MustGather` CR exists; deleting the CR can still trigger Kubernetes garbage collection of owned resources. |
🤖 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 `@README.md` at line 46, Update the README retention statement near “On CR
deletion” so it does not claim that retainResourcesOnCompletion preserves
resources after MustGather CR deletion. Limit the claim to completed Job
resources while the CR exists, or explicitly document that owned resources may
be garbage-collected after the CR and its finalizer are removed.
Contributor
|
@shivprakashmuley: 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. |
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
3 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.
Summary by CodeRabbit