OAPE-877: Standardize AGENTS.md and agent harness docs for ESO - #171
OAPE-877: Standardize AGENTS.md and agent harness docs for ESO#171bharath-b-rh wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references OAPE-877 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: bharath-b-rh 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds repository configuration, contributor templates, project guidance, architecture and domain documentation, testing and operational guidelines, security policy, decision records, and Markdown linting support for the External Secrets Operator. ChangesRepository governance and contributor entry points
Development, testing, and domain documentation
Markdown linting workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ai-docs/exec-plans/README.md (1)
3-20: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winComplete the execution-plan guide before exposing this index.
The document lists required topics, but it has no sections for them and
## Usageis empty. Contributors cannot learn when to create a plan, how to use it, or how to complete it. Add the missing guidance and a template link, or remove the placeholder entries until the guide exists.🤖 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/exec-plans/README.md` around lines 3 - 20, Complete the execution-plan guide in README by adding sections covering what exec-plans are, when to create them, how to use them, the completion workflow, and a link to the template; populate the existing Usage section rather than leaving it empty. Ensure every topic listed in the document index has corresponding contributor guidance before retaining the index.
🟡 Minor comments (18)
ai-docs/ESO_TESTING.md-18-24 (1)
18-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe
make testcorrectly.The Makefile target runs
manifests,generate,fmt,vet,test-apis, andtest-unit. It runs all non-E2E checks, not unit tests only.Change the comment or direct unit-test users to
make test-unit.Proposed correction
-make test # All unit tests +make test # All non-e2e tests🤖 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/ESO_TESTING.md` around lines 18 - 24, Update the `make test` entry in the testing command list to describe that it runs all non-E2E checks, including manifests, generate, fmt, vet, test-apis, and test-unit; reserve the “unit tests only” description for `make test-unit`.docs/testing-guidelines.md-5-10 (1)
5-10: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the table with executable commands.
The Makefile runs E2E tests from the
testmodule withgo test -C .../test, but the table showsgo test -tags e2e ./e2e, which fails when copied from the repository root. The unit-test entry also shows escaped pipes (\|) instead of the Makefile regex separators.Use copyable commands or describe the exclusions in words.
Proposed table corrections
-| `make test-unit` | `go test` excluding `test/(e2e\|apis\|utils)` | Standard Go tests | +| `make test-unit` | Go tests excluding E2E, API, and utility packages | Standard Go tests | -| `make test-e2e` | `go test -tags e2e ./e2e` | Requires live cluster | +| `make test-e2e` | `go test -C test -tags e2e ./e2e` | Requires live cluster |🤖 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 `@docs/testing-guidelines.md` around lines 5 - 10, Update the testing command table so each command matches the Makefile and can be copied from the repository root: correct the make test-e2e invocation to use the test module’s working directory via go test -C, and replace the escaped pipe characters in the make test-unit exclusion pattern with the actual Makefile syntax or describe those exclusions in words.ai-docs/domain/external-secrets-config.md-64-67 (1)
64-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winState the enabled condition for Bitwarden TLS validation.
The document defines
certManager.modeasEnabledorDisabled, but says only that cert-manager is “configured.” AcertProviderobject withmode: Disabledmust not satisfy the Bitwarden requirement. State the rule assecretReforcertManager.mode == Enabled.🤖 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/domain/external-secrets-config.md` around lines 64 - 67, Update the CEL Validation statement for Bitwarden TLS to explicitly require either secretRef or certManager.mode == Enabled, rather than treating any configured cert-manager object as sufficient.ai-docs/decisions/adr-template.md-20-35 (1)
20-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd blank lines after subsection headings.
markdownlint-cli2reports MD022 forPositive,Negative,Neutral, andAlternative 1. Insert one blank line after each heading.🤖 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/decisions/adr-template.md` around lines 20 - 35, Insert one blank line after each subsection heading in the ADR template: “Positive,” “Negative,” “Neutral,” and “Alternative 1,” before their associated content.Source: Linters/SAST tools
docs/api-contracts-guidelines.md-20-28 (1)
20-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse one error-matching contract for API tests.
Line 28 requires the exact error message, while line 177 requires an exact error substring. Choose one rule for all tests. Prefer stable substrings unless tests intentionally pin full API-server wording.
Also applies to: 175-177
🤖 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 `@docs/api-contracts-guidelines.md` around lines 20 - 28, Unify the API test error-matching guidance in the Field Immutability section and the corresponding onUpdate guidance around one contract: prefer asserting a stable error substring, reserving exact full-message matching for tests intentionally pinning API-server wording. Update the requirement for the exact error message and the guidance near onUpdate tests so they use the same rule.ai-docs/architecture/components.md-56-60 (1)
56-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow deletion as a terminal branch.
The sequence places
Add finalizerafterHandle deletion, which suggests that a deleted resource receives a finalizer and continues normal reconciliation. Show deletion ascleanup → remove finalizer → return; show the add-finalizer path only for non-deleted 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 `@ai-docs/architecture/components.md` around lines 56 - 60, Update the “Reconciliation Flow” diagram to make deletion a terminal branch: show deleted resources proceeding through cleanup and finalizer removal, then returning; show “Add finalizer → Fetch ESM → processReconcileRequest” only on the non-deleted path.ai-docs/domain/external-secrets-manager.md-44-45 (1)
44-45: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDocument RBAC ownership and the user-managed binding.
The operator reconciles
external-secrets-controllerand itsClusterRoleBindingfor theexternal-secretsServiceAccount in theexternal-secretsnamespace. That managed role does not grant write access to generic targets. State that users must create a separateRoleorClusterRoleand bind it to this ServiceAccount. Warn users not to edit the managed role because reconciliation restores its rules.🤖 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/domain/external-secrets-manager.md` around lines 44 - 45, Update the UnsafeAllowGenericTargets documentation to explain that the operator manages the external-secrets-controller and its ClusterRoleBinding for the external-secrets ServiceAccount in the external-secrets namespace, but the managed role does not grant generic-target write access. Instruct users to create a separate Role or ClusterRole and bind it to that ServiceAccount, and warn them not to edit the managed role because reconciliation restores its rules.ai-docs/decisions/adr-0002-update-with-retry-over-ssa.md-24-24 (1)
24-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd blank lines after ADR subsection headings.
Add one blank line after
### Positive,### Negative,### Neutral, and### Server-Side Apply. markdownlint reports MD022.As per coding guidelines,
**/*requiresmake verifybefore submission, and the gate checks formatting.Also applies to: 29-29, 33-33, 38-38
🤖 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/decisions/adr-0002-update-with-retry-over-ssa.md` at line 24, Add one blank line immediately after each ADR subsection heading: “Positive,” “Negative,” “Neutral,” and “Server-Side Apply,” ensuring the following content starts on the next separated paragraph to satisfy markdownlint MD022.Sources: Coding guidelines, Linters/SAST tools
docs/security-guidelines.md-47-54 (1)
47-54: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix blank lines around the security-context code fence.
Add a blank line before Line 47 and after Line 54. markdownlint reports MD031.
As per coding guidelines,
**/*requiresmake verifybefore submission, and the gate checks formatting.🤖 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 `@docs/security-guidelines.md` around lines 47 - 54, Add blank lines immediately before and after the security-context Go code fence in the documentation section, preserving the fenced content unchanged so markdownlint MD031 passes.Sources: Coding guidelines, Linters/SAST tools
ai-docs/decisions/adr-0001-bindata-over-helm.md-25-25 (1)
25-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd blank lines after ADR subsection headings.
Add one blank line after
### Positive,### Negative,### Neutral,### Direct Helm Library Usage, and### Kustomize Overlays. markdownlint reports MD022.As per coding guidelines,
**/*requiresmake verifybefore submission, and the gate checks formatting.Also applies to: 30-30, 34-34, 39-39, 43-43
🤖 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/decisions/adr-0001-bindata-over-helm.md` at line 25, Add one blank line immediately after each ADR subsection heading: Positive, Negative, Neutral, Direct Helm Library Usage, and Kustomize Overlays, so the document satisfies markdownlint MD022 and the formatting verification gate.Sources: Coding guidelines, Linters/SAST tools
docs/error-handling-guidelines.md-7-7 (1)
7-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the reported Markdown spacing violations.
Add blank lines after the headings at Lines 7, 21, 28, 86, 89, 92, and 106. Add a blank line before the code fence at Line 116. markdownlint reports MD022 and MD031.
As per coding guidelines,
**/*requiresmake verifybefore submission, and the gate checks formatting.Also applies to: 21-21, 28-28, 86-86, 89-89, 92-92, 106-106, 116-116
🤖 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 `@docs/error-handling-guidelines.md` at line 7, Fix the Markdown spacing violations in docs/error-handling-guidelines.md by adding blank lines after the headings at the referenced locations and before the code fence at line 116, satisfying markdownlint rules MD022 and MD031. Run make verify to confirm formatting passes.Sources: Coding guidelines, Linters/SAST tools
ai-docs/decisions/adr-0003-network-policy-naming-scheme.md-28-28 (1)
28-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd blank lines after ADR subsection headings.
Add one blank line after
### Positive,### Negative, and### Neutral. markdownlint reports MD022.As per coding guidelines,
**/*requiresmake verifybefore submission, and the gate checks formatting.Also applies to: 33-33, 37-37
🤖 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/decisions/adr-0003-network-policy-naming-scheme.md` at line 28, Add a blank line immediately after the ADR subsection headings “Positive,” “Negative,” and “Neutral” so each heading is separated from its following content and satisfies markdownlint MD022.Sources: Coding guidelines, Linters/SAST tools
docs/integration-guidelines.md-57-61 (1)
57-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpecify a language for the manifest tree fence.
Use
textafter the fence at Line 57. markdownlint reports MD040.As per coding guidelines,
**/*requiresmake verifybefore submission, and the gate checks formatting.Suggested fix
-``` +```text🤖 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 `@docs/integration-guidelines.md` around lines 57 - 61, Specify the text language on the manifest tree code fence in the documentation by changing the opening fence to use the text tag, while preserving the existing tree content and formatting.Sources: Coding guidelines, Linters/SAST tools
ai-docs/decisions/adr-0003-network-policy-naming-scheme.md-38-38 (1)
38-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the maximum user-policy name length.
eso-user-has 9 characters. With Kubernetes' 253-character name limit, the base name can be 244 characters, not 243. If 243 reserves another suffix, state that suffix.Suggested fix
-- User policy names are limited to 243 characters to accommodate the `eso-user-` prefix within Kubernetes' 253-character name limit +- User policy names are limited to 244 characters to accommodate the `eso-user-` prefix within Kubernetes' 253-character name limit🤖 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/decisions/adr-0003-network-policy-naming-scheme.md` at line 38, Update the maximum user policy name length in the naming scheme documentation from 243 to 244 characters to account for the 9-character eso-user- prefix within Kubernetes’ 253-character limit; only retain 243 if the documentation explicitly identifies an additional reserved suffix.docs/error-handling-guidelines.md-5-5 (1)
5-5: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winWrap CRD annotator failures with
ReconcileErrorbefore aggregation.processReconcileRequestreturns bare errors throughoErr, and its status-update path aggregates errors without adding a category.utilerrors.NewAggregatesupportserrors.As, but it cannot classify bare errors. UseFromClientErrororNewRetryRequiredError, or document the CRD annotator exception. Extend the test to assert the error category, not onlyerr.Error().🤖 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 `@docs/error-handling-guidelines.md` at line 5, Update processReconcileRequest so CRD annotator failures assigned to oErr are wrapped with an appropriate ReconcileError category, using FromClientError or NewRetryRequiredError, before status-update aggregation via utilerrors.NewAggregate. Preserve the existing aggregation behavior and extend the related test to assert the resulting error category through errors.As rather than checking only err.Error()..github/PULL_REQUEST_TEMPLATE.md-23-25 (1)
23-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign all
make verifydocumentation with the Makefile contract.The supplied Makefile does not run lint as part of
verify, and its git-diff check is not limited to generated files.
- .github/PULL_REQUEST_TEMPLATE.md#L23-L25: Remove
no lint errorsfrom themake verifyitem; keepmake lintas the separate check.- CONTRIBUTING.md#L89-L89: Tell contributors to inspect the verify diff because it can include uncommitted or untracked changes, then regenerate only when generated outputs changed.
🤖 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 @.github/PULL_REQUEST_TEMPLATE.md around lines 23 - 25, Update .github/PULL_REQUEST_TEMPLATE.md lines 23-25 by removing “no lint errors” from the make verify checklist while keeping make lint as a separate check. Update CONTRIBUTING.md line 89 to instruct contributors to inspect the verify diff, including uncommitted or untracked changes, and regenerate outputs only when generated files changed.CONTRIBUTING.md-61-63 (1)
61-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse an imperative no-ticket commit example.
updates owners listis not imperative. Replace it withupdate owners list.As per coding guidelines, use a descriptive imperative summary when no Jira ticket exists.
Proposed wording
-The Jira project can be any valid project (ESO, OAPE, etc.). For changes without a Jira ticket, use a descriptive imperative summary (e.g., `fix make verify`, `updates owners list`). +The Jira project can be any valid project (ESO, OAPE, etc.). For changes without a Jira ticket, use a descriptive imperative summary (e.g., `fix make verify`, `update owners list`).🤖 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` around lines 61 - 63, Update the no-ticket commit example in the contributing guidance from “updates owners list” to the imperative “update owners list,” while preserving the surrounding examples and wording.Source: Coding guidelines
README.md-68-70 (1)
68-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSeparate Podman and Docker prerequisites.
CONTAINER_TOOLdefaults to Podman, but the repository defines no Podman minimum. Docker 17.03 cannot build the multi-stageDockerfile; basic builds require Docker 17.05+, anddocker-buildxrequires Docker 19.03+. List separate, build-script-supported requirements.🤖 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` around lines 68 - 70, Update the prerequisites section in README.md to separate Podman and Docker requirements, documenting the repository’s supported Podman baseline and Docker minimums for basic multi-stage builds and docker-buildx. Keep the existing Go and kubectl/oc prerequisites unchanged, and align Docker versions with the build scripts’ actual requirements.
🧹 Nitpick comments (7)
ai-docs/ESO_TESTING.md (1)
42-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse unique names for repeated headings.
### Runningappears twice. markdownlint-cli2 reports MD024. Rename the headings to### Running API Testsand### Running E2E Tests.Also applies to: 66-76
🤖 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/ESO_TESTING.md` around lines 42 - 47, Rename the duplicate “### Running” headings in ai-docs/ESO_TESTING.md to unique headings: use “### Running API Tests” for the API test section and “### Running E2E Tests” for the end-to-end test section, preserving their existing content.Source: Linters/SAST tools
ai-docs/ESO_DEVELOPMENT.md (1)
50-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid a hard-coded “current” version.
The
currently v2.5.0text can drift fromEXTERNAL_SECRETS_VERSIONinMakefile. Remove the parenthetical or add an automated documentation check.🤖 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/ESO_DEVELOPMENT.md` around lines 50 - 61, Remove the hard-coded “currently v2.5.0” parenthetical from the version-update step in the ESO development instructions, leaving the Makefile variable as the sole source of truth.docs/testing-guidelines.md (2)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the Markdown lint violations.
Add blank lines before and after the fenced block at lines 17-19. Specify
bashas the fence language. Add blank lines around### Do Testand### Do Not Test.markdownlint-cli2 reports MD031, MD040, and MD022 for these ranges.
Also applies to: 108-119
🤖 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 `@docs/testing-guidelines.md` around lines 16 - 20, Update the Markdown structure around the testing guidelines and the corresponding section near “Do Test”/“Do Not Test”: add blank lines before and after every fenced code block, declare the fence language as bash, and add blank lines before and after each affected level-three heading to satisfy MD031, MD040, and MD022.Source: Linters/SAST tools
84-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
//go:embed testdata/*inai-docs/ESO_TESTING.md. The declaration intest/e2e/e2e_test.gouses this pattern, so both testing guides must match.🤖 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 `@docs/testing-guidelines.md` around lines 84 - 87, Update the Embedded Testdata guidance in the testing documentation to specify //go:embed testdata/* for ai-docs/ESO_TESTING.md, matching the declaration used by e2e_test.go while preserving the existing testassets.ReadFile and ReplacePatternInAsset guidance.ai-docs/references/ecosystem.md (1)
40-40: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd the enforcement reference and scope.
Reference
cmd/external-secrets-operator/main.go:135-156. State that HTTP/2 is disabled by default for the metrics and webhook servers and enabled when--enable-http2is set.🤖 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 40, Update the HTTP/2 entry in ecosystem.md to reference the enforcement in the operator’s main server configuration and specify that HTTP/2 is disabled by default for both metrics and webhook servers, becoming enabled when --enable-http2 is set.ai-docs/domain/external-secrets-manager.md (1)
61-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument recovery after runtime ESM deletion.
CreateDefaultESMResourceruns only during operator startup. Runtime deletion removes the finalizer but does not recreateExternalSecretsManager. Document that administrators must restart the operator, for example withoc rollout restart deployment/external-secrets-operator-controller-manager -n external-secrets-operator, to trigger recreation.🤖 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/domain/external-secrets-manager.md` around lines 61 - 65, Update the Lifecycle documentation for ExternalSecretsManager deletion to state that runtime deletion is not automatically recovered because CreateDefaultESMResource runs only at operator startup; instruct administrators to restart the operator, including the provided oc rollout restart command, to trigger recreation.docs/integration-guidelines.md (1)
9-13: 🩺 Stability & Availability | 🔵 TrivialDocument recovery after late cert-manager installation.
If cert-manager is installed after the operator starts, the cached discovery result remains negative and the documented
IrrecoverableErrorpath does not self-recover. State that operators must restart the operator, or change the detection and retry behavior. Verify that restart is the supported recovery 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 `@docs/integration-guidelines.md` around lines 9 - 13, Update the cert-manager recovery guidance near isCRDInstalled, optionalResourcesList, and NewIrrecoverableError to state that operators must restart the operator after installing cert-manager late, since detection is cached for the process lifetime. Preserve the existing startup-only detection behavior and document restart as the supported recovery path rather than implying automatic recovery.
🤖 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 @.github/ISSUE_TEMPLATE/bug_report.md:
- Around line 32-46: Update the bug report template before the “Relevant
Resources” and “Operator Logs” sections to add a prominent warning instructing
users to redact secret values, credentials, tokens, private keys, and personal
data from all diagnostics before posting publicly.
In @.github/PULL_REQUEST_TEMPLATE.md:
- Around line 26-29: Update the checklist entries in the pull request template
to match the contributor workflow: qualify CEL validation rules and tests as
required only for applicable fields or new rules, and expand the
managed-resource checklist to include registration in
controllerManagedResources, HasObjectChanged, buildCacheObjectList(), and the
required ordered install sequence. Preserve the existing generated-file and
error-type checklist items.
In `@ai-docs/architecture/components.md`:
- Around line 120-129: Update the “Update Strategy: NOT Server-Side Apply”
section to scope the UpdateWithRetry statement specifically to full-resource
updates, while explicitly distinguishing metadata-only updates via
patchResourceMetadata and annotation updates via MergePatch. Keep the existing
references and strategy table consistent with this distinction.
- Around line 93-102: Update the “Two Clients” invariant near the
controllerManagedResources guidance to explicitly allow r.UncachedClient only as
a fallback when a controller-managed object is absent from the cache, while
requiring r.CtrlClient for normal access to cached controller-managed resources.
Ensure the cached and uncached client rules are consistent and no longer
contradictory.
In `@ai-docs/decisions/adr-0003-network-policy-naming-scheme.md`:
- Around line 33-35: Update the network-policy naming scheme ADR to document the
supported process for revoking obsolete permissions when entries cannot be
removed and missing eso-user-* policies are recreated, and state that any change
to permit removal requires an enhancement proposal before modifying the CEL
immutability rule.
In `@ai-docs/domain/external-secrets-config.md`:
- Around line 90-94: Update the Lifecycle deletion item near the “Deletion”
entry to document cleanup of both namespace-scoped and cluster-scoped
operator-managed resources, including cluster-scoped RBAC, validating webhooks,
and tracked CRD annotations. Explicitly describe any resources or conditions
that are exceptions to cleanup, while preserving the existing
finalizer-protected behavior.
In `@ai-docs/ESO_DEVELOPMENT.md`:
- Around line 71-81: Update the managed-resource workflow in “Add a New Managed
Resource Type” to run make update-bindata immediately after adding YAML under
bindata/external-secrets/resources/, then run make verify to confirm generated
assets are current. Preserve the existing steps and also note that CRD or other
generated-input changes require make manifests generate update-bindata.
- Around line 149-151: Update the FIPS section of ESO_DEVELOPMENT.md to
distinguish the hack/go-fips.sh path used by make build-operator from the
image-build and docker-buildx targets using Dockerfile with CGO_ENABLED=0 and no
FIPS flags. State that CGO_ENABLED=0 applies to non-FIPS image builds, without
implying those targets produce FIPS-compliant images.
In `@docs/api-contracts-guidelines.md`:
- Around line 153-159: The Test Patterns example uses inconsistent CRD
identifiers: align the test directory path and the YAML crdName with the same
actual CRD name, updating the singular/plural example as needed while preserving
the surrounding test layout guidance.
In `@docs/performance-guidelines.md`:
- Line 63: Update the `FromClientError` classification statement in the
performance guidelines to explicitly exclude user-configuration `NotFound`
errors from the “everything else” `RetryRequiredError` rule, documenting that
they become `UserConfigurationError` with the status and requeue behavior
defined in the error-handling guidelines.
- Around line 9-18: Update the first cache rule in the performance guidelines to
explicitly exclude ConfigMaps from the label-filtered cache requirement. State
that operand types in controllerManagedResources use the app=external-secrets
selector except ConfigMaps, which use namespace scoping via
OperandDefaultNamespace and must not receive a label selector; keep the existing
own-CR and unbounded-resource guidance unchanged.
---
Outside diff comments:
In `@ai-docs/exec-plans/README.md`:
- Around line 3-20: Complete the execution-plan guide in README by adding
sections covering what exec-plans are, when to create them, how to use them, the
completion workflow, and a link to the template; populate the existing Usage
section rather than leaving it empty. Ensure every topic listed in the document
index has corresponding contributor guidance before retaining the index.
---
Minor comments:
In @.github/PULL_REQUEST_TEMPLATE.md:
- Around line 23-25: Update .github/PULL_REQUEST_TEMPLATE.md lines 23-25 by
removing “no lint errors” from the make verify checklist while keeping make lint
as a separate check. Update CONTRIBUTING.md line 89 to instruct contributors to
inspect the verify diff, including uncommitted or untracked changes, and
regenerate outputs only when generated files changed.
In `@ai-docs/architecture/components.md`:
- Around line 56-60: Update the “Reconciliation Flow” diagram to make deletion a
terminal branch: show deleted resources proceeding through cleanup and finalizer
removal, then returning; show “Add finalizer → Fetch ESM →
processReconcileRequest” only on the non-deleted path.
In `@ai-docs/decisions/adr-0001-bindata-over-helm.md`:
- Line 25: Add one blank line immediately after each ADR subsection heading:
Positive, Negative, Neutral, Direct Helm Library Usage, and Kustomize Overlays,
so the document satisfies markdownlint MD022 and the formatting verification
gate.
In `@ai-docs/decisions/adr-0002-update-with-retry-over-ssa.md`:
- Line 24: Add one blank line immediately after each ADR subsection heading:
“Positive,” “Negative,” “Neutral,” and “Server-Side Apply,” ensuring the
following content starts on the next separated paragraph to satisfy markdownlint
MD022.
In `@ai-docs/decisions/adr-0003-network-policy-naming-scheme.md`:
- Line 28: Add a blank line immediately after the ADR subsection headings
“Positive,” “Negative,” and “Neutral” so each heading is separated from its
following content and satisfies markdownlint MD022.
- Line 38: Update the maximum user policy name length in the naming scheme
documentation from 243 to 244 characters to account for the 9-character
eso-user- prefix within Kubernetes’ 253-character limit; only retain 243 if the
documentation explicitly identifies an additional reserved suffix.
In `@ai-docs/decisions/adr-template.md`:
- Around line 20-35: Insert one blank line after each subsection heading in the
ADR template: “Positive,” “Negative,” “Neutral,” and “Alternative 1,” before
their associated content.
In `@ai-docs/domain/external-secrets-config.md`:
- Around line 64-67: Update the CEL Validation statement for Bitwarden TLS to
explicitly require either secretRef or certManager.mode == Enabled, rather than
treating any configured cert-manager object as sufficient.
In `@ai-docs/domain/external-secrets-manager.md`:
- Around line 44-45: Update the UnsafeAllowGenericTargets documentation to
explain that the operator manages the external-secrets-controller and its
ClusterRoleBinding for the external-secrets ServiceAccount in the
external-secrets namespace, but the managed role does not grant generic-target
write access. Instruct users to create a separate Role or ClusterRole and bind
it to that ServiceAccount, and warn them not to edit the managed role because
reconciliation restores its rules.
In `@ai-docs/ESO_TESTING.md`:
- Around line 18-24: Update the `make test` entry in the testing command list to
describe that it runs all non-E2E checks, including manifests, generate, fmt,
vet, test-apis, and test-unit; reserve the “unit tests only” description for
`make test-unit`.
In `@CONTRIBUTING.md`:
- Around line 61-63: Update the no-ticket commit example in the contributing
guidance from “updates owners list” to the imperative “update owners list,”
while preserving the surrounding examples and wording.
In `@docs/api-contracts-guidelines.md`:
- Around line 20-28: Unify the API test error-matching guidance in the Field
Immutability section and the corresponding onUpdate guidance around one
contract: prefer asserting a stable error substring, reserving exact
full-message matching for tests intentionally pinning API-server wording. Update
the requirement for the exact error message and the guidance near onUpdate tests
so they use the same rule.
In `@docs/error-handling-guidelines.md`:
- Line 7: Fix the Markdown spacing violations in
docs/error-handling-guidelines.md by adding blank lines after the headings at
the referenced locations and before the code fence at line 116, satisfying
markdownlint rules MD022 and MD031. Run make verify to confirm formatting
passes.
- Line 5: Update processReconcileRequest so CRD annotator failures assigned to
oErr are wrapped with an appropriate ReconcileError category, using
FromClientError or NewRetryRequiredError, before status-update aggregation via
utilerrors.NewAggregate. Preserve the existing aggregation behavior and extend
the related test to assert the resulting error category through errors.As rather
than checking only err.Error().
In `@docs/integration-guidelines.md`:
- Around line 57-61: Specify the text language on the manifest tree code fence
in the documentation by changing the opening fence to use the text tag, while
preserving the existing tree content and formatting.
In `@docs/security-guidelines.md`:
- Around line 47-54: Add blank lines immediately before and after the
security-context Go code fence in the documentation section, preserving the
fenced content unchanged so markdownlint MD031 passes.
In `@docs/testing-guidelines.md`:
- Around line 5-10: Update the testing command table so each command matches the
Makefile and can be copied from the repository root: correct the make test-e2e
invocation to use the test module’s working directory via go test -C, and
replace the escaped pipe characters in the make test-unit exclusion pattern with
the actual Makefile syntax or describe those exclusions in words.
In `@README.md`:
- Around line 68-70: Update the prerequisites section in README.md to separate
Podman and Docker requirements, documenting the repository’s supported Podman
baseline and Docker minimums for basic multi-stage builds and docker-buildx.
Keep the existing Go and kubectl/oc prerequisites unchanged, and align Docker
versions with the build scripts’ actual requirements.
---
Nitpick comments:
In `@ai-docs/domain/external-secrets-manager.md`:
- Around line 61-65: Update the Lifecycle documentation for
ExternalSecretsManager deletion to state that runtime deletion is not
automatically recovered because CreateDefaultESMResource runs only at operator
startup; instruct administrators to restart the operator, including the provided
oc rollout restart command, to trigger recreation.
In `@ai-docs/ESO_DEVELOPMENT.md`:
- Around line 50-61: Remove the hard-coded “currently v2.5.0” parenthetical from
the version-update step in the ESO development instructions, leaving the
Makefile variable as the sole source of truth.
In `@ai-docs/ESO_TESTING.md`:
- Around line 42-47: Rename the duplicate “### Running” headings in
ai-docs/ESO_TESTING.md to unique headings: use “### Running API Tests” for the
API test section and “### Running E2E Tests” for the end-to-end test section,
preserving their existing content.
In `@ai-docs/references/ecosystem.md`:
- Line 40: Update the HTTP/2 entry in ecosystem.md to reference the enforcement
in the operator’s main server configuration and specify that HTTP/2 is disabled
by default for both metrics and webhook servers, becoming enabled when
--enable-http2 is set.
In `@docs/integration-guidelines.md`:
- Around line 9-13: Update the cert-manager recovery guidance near
isCRDInstalled, optionalResourcesList, and NewIrrecoverableError to state that
operators must restart the operator after installing cert-manager late, since
detection is cached for the process lifetime. Preserve the existing startup-only
detection behavior and document restart as the supported recovery path rather
than implying automatic recovery.
In `@docs/testing-guidelines.md`:
- Around line 16-20: Update the Markdown structure around the testing guidelines
and the corresponding section near “Do Test”/“Do Not Test”: add blank lines
before and after every fenced code block, declare the fence language as bash,
and add blank lines before and after each affected level-three heading to
satisfy MD031, MD040, and MD022.
- Around line 84-87: Update the Embedded Testdata guidance in the testing
documentation to specify //go:embed testdata/* for ai-docs/ESO_TESTING.md,
matching the declaration used by e2e_test.go while preserving the existing
testassets.ReadFile and ReplacePatternInAsset guidance.
🪄 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: 6528e29c-5b6a-4fec-8df9-80c195ac9b98
📒 Files selected for processing (27)
.coderabbit.yaml.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.mdAGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mdSECURITY.mdai-docs/ESO_DEVELOPMENT.mdai-docs/ESO_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-bindata-over-helm.mdai-docs/decisions/adr-0002-update-with-retry-over-ssa.mdai-docs/decisions/adr-0003-network-policy-naming-scheme.mdai-docs/decisions/adr-template.mdai-docs/domain/external-secrets-config.mdai-docs/domain/external-secrets-manager.mdai-docs/exec-plans/README.mdai-docs/references/ecosystem.mdai-docs/references/enhancements.mddocs/api-contracts-guidelines.mddocs/error-handling-guidelines.mddocs/integration-guidelines.mddocs/performance-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.md
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
ai-docs/exec-plans/README.md (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the archive location.
The documented layout contains only
exec-plans/active/, but this line also tells contributors to archive completed plans. Add and document an archive directory, or instruct contributors to delete completed plans.🤖 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/exec-plans/README.md` at line 22, Update the plan-completion guidance in the README to resolve the inconsistency between the documented exec-plans layout and the instruction to archive completed plans. Either document a dedicated archive directory alongside exec-plans/active/ and direct contributors to use it, or change the guidance to require deleting completed plans.
🤖 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 `@docs/anti-patterns/DUAL_CACHE_FIX.md`:
- Around line 136-141: Replace the fixed cache savings claims in the “Reduces
Resource Usage” section with workload-dependent language or measured
before-and-after results. Remove the unsupported 2N-to-N watch-connection and
cached-object calculations and the approximate 50% savings claims, while
accurately distinguishing informer watch connections from cached object counts.
In `@hack/Dockerfile.markdownlint`:
- Around line 7-8: Update the MARKDOWNLINT_BASE_IMAGE declaration in the
Dockerfile to use an approved immutable digest reference. If retaining the
build-argument override, ensure the build flow passes it explicitly and rejects
any tag-only or otherwise unpinned value.
- Line 18: Add USER 1000 to hack/Dockerfile.markdownlint and update both
Markdownlint container commands to pass --user "$$(id -u):$$(id -g)", ensuring
linting runs as a non-root user and --fix can write to bind-mounted files.
- Around line 12-18: Refactor the markdownlint Dockerfile into builder and final
stages: install markdownlint-cli2 only in the builder, then copy the Node
runtime and required package files into the final image while excluding npm,
caches, and installation artifacts. Pin the non-Red Hat base image by digest,
configure a non-root USER, and add the required HEALTHCHECK; preserve the
markdownlint entrypoint behavior.
In `@Makefile`:
- Around line 245-250: Define the MARKDOWNLINT_BASE_IMAGE Make variable and
update the markdownlint-image target’s CONTAINER_TOOL build invocation to pass
it via --build-arg MARKDOWNLINT_BASE_IMAGE=$(MARKDOWNLINT_BASE_IMAGE).
- Around line 252-255: Update the lint-markdown target to mount $(PROJECT_ROOT)
read-only, add the container --read-only option, and provide a writable /tmp
tmpfs; leave lint-markdown-fix writable because it runs markdownlint with --fix.
---
Nitpick comments:
In `@ai-docs/exec-plans/README.md`:
- Line 22: Update the plan-completion guidance in the README to resolve the
inconsistency between the documented exec-plans layout and the instruction to
archive completed plans. Either document a dedicated archive directory alongside
exec-plans/active/ and direct contributors to use it, or change the guidance to
require deleting completed plans.
🪄 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: 7cebfab0-6309-443a-ae4c-b01943b6a1f3
📒 Files selected for processing (32)
.github/ISSUE_TEMPLATE/bug_report.md.github/PULL_REQUEST_TEMPLATE.md.markdownlint-cli2.yaml.markdownlint.yamlAGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefileREADME.mdai-docs/ESO_DEVELOPMENT.mdai-docs/ESO_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-bindata-over-helm.mdai-docs/decisions/adr-0002-update-with-retry-over-ssa.mdai-docs/decisions/adr-0003-network-policy-naming-scheme.mdai-docs/decisions/adr-template.mdai-docs/domain/external-secrets-config.mdai-docs/domain/external-secrets-manager.mdai-docs/exec-plans/README.mddocs/anti-patterns/DUAL_CACHE_FIX.mddocs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.mddocs/anti-patterns/README.mddocs/api-contracts-guidelines.mddocs/error-handling-guidelines.mddocs/integration-guidelines.mddocs/performance-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.mdhack/Dockerfile.markdownlinthack/install-markdownlint.shhack/markdownlint.shtest/e2e/README.md
💤 Files with no reviewable changes (1)
- .markdownlint.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- AGENTS.md
- CONTRIBUTING.md
- ai-docs/domain/external-secrets-manager.md
- ai-docs/architecture/components.md
- ai-docs/decisions/adr-template.md
- ai-docs/ESO_TESTING.md
- CLAUDE.md
- ai-docs/decisions/adr-0001-bindata-over-helm.md
- ai-docs/domain/external-secrets-config.md
- docs/error-handling-guidelines.md
- ai-docs/decisions/adr-0003-network-policy-naming-scheme.md
- docs/testing-guidelines.md
- docs/performance-guidelines.md
- docs/integration-guidelines.md
- README.md
- docs/security-guidelines.md
- ai-docs/decisions/adr-0002-update-with-retry-over-ssa.md
|
/test verify |
There was a problem hiding this comment.
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 `@Makefile`:
- Around line 287-297: The unconditional exit 1 statement after the retry build
command causes the target to fail even when the retry succeeds. Replace the
unconditional exit 1 with exit $? to propagate the retry command's status, so
successful recoveries result in success and only actual failures trigger the
exit code 1.
🪄 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: 4fe5e53e-2d7f-4546-ab23-d4eafea432a8
📒 Files selected for processing (32)
.github/ISSUE_TEMPLATE/bug_report.md.github/PULL_REQUEST_TEMPLATE.md.markdownlint-cli2.yaml.markdownlint.yamlAGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefileREADME.mdai-docs/ESO_DEVELOPMENT.mdai-docs/ESO_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-bindata-over-helm.mdai-docs/decisions/adr-0002-update-with-retry-over-ssa.mdai-docs/decisions/adr-0003-network-policy-naming-scheme.mdai-docs/decisions/adr-template.mdai-docs/domain/external-secrets-config.mdai-docs/domain/external-secrets-manager.mdai-docs/exec-plans/README.mddocs/anti-patterns/DUAL_CACHE_FIX.mddocs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.mddocs/anti-patterns/README.mddocs/api-contracts-guidelines.mddocs/error-handling-guidelines.mddocs/integration-guidelines.mddocs/performance-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.mdhack/Dockerfile.markdownlinthack/install-markdownlint.shhack/markdownlint.shtest/e2e/README.md
💤 Files with no reviewable changes (1)
- .markdownlint.yaml
🚧 Files skipped from review as they are similar to previous changes (29)
- ai-docs/decisions/adr-0003-network-policy-naming-scheme.md
- docs/error-handling-guidelines.md
- .markdownlint-cli2.yaml
- .github/PULL_REQUEST_TEMPLATE.md
- CONTRIBUTING.md
- hack/install-markdownlint.sh
- docs/security-guidelines.md
- ai-docs/decisions/adr-template.md
- ai-docs/domain/external-secrets-config.md
- ai-docs/decisions/adr-0001-bindata-over-helm.md
- CLAUDE.md
- ai-docs/decisions/adr-0002-update-with-retry-over-ssa.md
- hack/markdownlint.sh
- ai-docs/architecture/components.md
- ai-docs/domain/external-secrets-manager.md
- .github/ISSUE_TEMPLATE/bug_report.md
- ai-docs/exec-plans/README.md
- ai-docs/ESO_TESTING.md
- ai-docs/ESO_DEVELOPMENT.md
- docs/performance-guidelines.md
- test/e2e/README.md
- docs/integration-guidelines.md
- docs/anti-patterns/DUAL_CACHE_FIX.md
- hack/Dockerfile.markdownlint
- README.md
- docs/api-contracts-guidelines.md
- docs/testing-guidelines.md
- AGENTS.md
- docs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/Dockerfile.markdownlint (1)
1-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
HEALTHCHECKinstruction.The digest pin, multi-stage build, and non-root
USER 1000:1000from the prior review are now in place. One path-instruction item remains unaddressed: the Dockerfile has noHEALTHCHECKinstruction. This image runs as a one-shot CLI wrapper invoked bymake lint-markdown, so the practical benefit is limited, but add aHEALTHCHECK(for example, checkingmarkdownlint-cli2 --versionsucceeds) to satisfy the container security checklist, or document why it is intentionally omitted for a non-service CLI image.🤖 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 `@hack/Dockerfile.markdownlint` around lines 1 - 36, Add a Dockerfile HEALTHCHECK after the markdownlint CLI setup that verifies markdownlint-cli2 is available by successfully running its version command; keep it compatible with the non-root runtime user and one-shot ENTRYPOINT.Source: Path instructions
🤖 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.
Nitpick comments:
In `@hack/Dockerfile.markdownlint`:
- Around line 1-36: Add a Dockerfile HEALTHCHECK after the markdownlint CLI
setup that verifies markdownlint-cli2 is available by successfully running its
version command; keep it compatible with the non-root runtime user and one-shot
ENTRYPOINT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dd258c49-8723-43a9-b0bf-707f44a25e5b
📒 Files selected for processing (32)
.github/ISSUE_TEMPLATE/bug_report.md.github/PULL_REQUEST_TEMPLATE.md.markdownlint-cli2.yaml.markdownlint.yamlAGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefileREADME.mdai-docs/ESO_DEVELOPMENT.mdai-docs/ESO_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-bindata-over-helm.mdai-docs/decisions/adr-0002-update-with-retry-over-ssa.mdai-docs/decisions/adr-0003-network-policy-naming-scheme.mdai-docs/decisions/adr-template.mdai-docs/domain/external-secrets-config.mdai-docs/domain/external-secrets-manager.mdai-docs/exec-plans/README.mddocs/anti-patterns/DUAL_CACHE_FIX.mddocs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.mddocs/anti-patterns/README.mddocs/api-contracts-guidelines.mddocs/error-handling-guidelines.mddocs/integration-guidelines.mddocs/performance-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.mdhack/Dockerfile.markdownlinthack/install-markdownlint.shhack/markdownlint.shtest/e2e/README.md
💤 Files with no reviewable changes (1)
- .markdownlint.yaml
🚧 Files skipped from review as they are similar to previous changes (27)
- test/e2e/README.md
- docs/error-handling-guidelines.md
- ai-docs/exec-plans/README.md
- ai-docs/decisions/adr-template.md
- docs/performance-guidelines.md
- docs/security-guidelines.md
- .markdownlint-cli2.yaml
- ai-docs/domain/external-secrets-config.md
- README.md
- CLAUDE.md
- ai-docs/decisions/adr-0003-network-policy-naming-scheme.md
- docs/api-contracts-guidelines.md
- .github/ISSUE_TEMPLATE/bug_report.md
- docs/anti-patterns/DUAL_CACHE_FIX.md
- ai-docs/domain/external-secrets-manager.md
- .github/PULL_REQUEST_TEMPLATE.md
- ai-docs/architecture/components.md
- ai-docs/decisions/adr-0001-bindata-over-helm.md
- docs/anti-patterns/DUAL_CACHE_FIX_SUMMARY.md
- AGENTS.md
- ai-docs/decisions/adr-0002-update-with-retry-over-ssa.md
- ai-docs/ESO_TESTING.md
- ai-docs/ESO_DEVELOPMENT.md
- CONTRIBUTING.md
- docs/integration-guidelines.md
- docs/testing-guidelines.md
- hack/markdownlint.sh
c92cf2b to
509eb65
Compare
|
/testwith openshift/external-secrets-operator/main/verify openshift/release#82965 |
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
|
@bharath-b-rh: 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. |
2 participants
Description
What changed?
AGENTS.md,ai-docs/) for ESO architecture, domain CRs, ADRs, and development/testing workflows.docs/*-guidelines.md),CLAUDE.md, contributor/security docs, GitHub PR/issue templates, and CodeRabbit config wired to those guidelines.README.mdto point at the new docs layout.Why?
Standardize ESO agent/harness documentation so AI-assisted and human contributors share one entry point (
AGENTS.md) with clear split between enforceable guidelines (docs/) and deeper component docs (ai-docs/), aligned with OpenShift agentic-docs conventions.How?
agentic-docs/component-docsas the spine.CLAUDE.md, README/CONTRIBUTING/SECURITY, CodeRabbit, GitHub templates) without duplicating architecture into a second system.AGENTS.mdindexes bothdocs/andai-docs/; API/design changes inCONTRIBUTING.mdpoint atopenshift/enhancements/.../external-secrets-operator.Type of Change
Checklist
make verifypasses (generated files up to date, deps consistent, no lint errors)make testpasses (unit + API integration tests)make lintpasses.testsuite.yamltest casescontrollerManagedResourcesandHasObjectChangedbindata.go,zz_generated.deepcopy.go, CRD YAML, fakes)IrrecoverableError/RetryRequiredError/UserConfigurationError)Testing
Docs-only change. No runtime/code behavior changes. Spot-checked links and doc structure;
make verify/make test/make lintN/A or unchanged for functional code.Additional Context
Summary by CodeRabbit