Adds agents.md for External-Secrets Operator#150
Open
siddhibhor-56 wants to merge 1 commit into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR introduces eight comprehensive documentation files establishing operational, API design, error handling, integration, performance, security, and testing guidelines for the external-secrets-operator. The docs define conventions for CRD validation, reconciliation patterns, platform integrations, and multi-tier test strategy. ChangesOperator Documentation Guideline Suite
🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (5)
docs/testing-guidelines.md (1)
17-113: 💤 Low valueConsider adding blank lines around headings for Markdown compliance.
Multiple headings violate MD022 (blanks-around-headings). While this is a minor formatting issue, adding blank lines improves readability and passes linting. Given the "Chill" review preference, this is optional.
Example:
## Unit Tests (`pkg/`) ### Conventions + - Files live alongside source in the same package (e.g., `deployments.go` / `deployments_test.go`).🤖 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 17 - 113, The Markdown file has multiple headings without surrounding blank lines (violating MD022); fix by inserting a single blank line before and after each heading token (e.g., the "### Conventions", "### Fake Client Pattern", "### Test Helpers", "### Error Assertion", "### Adding a New Unit Test", "## API Integration Tests (`test/apis/`)", "### Data-Driven Test Suites", "### envtest Setup", "### Running", "### Adding a New API Test", "## E2E Tests (`test/e2e/`)", "### Build Tag", "### Label-Based Platform Selection", "### E2E Structure", "### Test Data", "### E2E Test Utilities (`test/utils/`)", "### Adding a New E2E Test") so each heading is preceded and followed by exactly one blank line to satisfy lint MD022 and improve readability.CLAUDE.md (1)
9-51: ⚡ Quick winAdd blank lines around headings and code fences for Markdown compliance.
Multiple sections violate MD022 (blanks-around-headings) and MD031 (blanks-around-fences). While the content renders correctly in most viewers, proper spacing improves readability and passes linting.
📐 Example fix for one section
### Before Committing + ```bash make update && make verify
This regenerates all code, manifests, and bindata, then runs verification checks. CI will reject PRs with stale generated files.
Apply similar spacing to all other sections (Development Workflow, Testing, Dependency Management, Claude Code Preferences). </details> <details> <summary>🤖 Prompt for AI Agents</summary>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.mdaround lines 9 - 51, Add a blank line before and after each
Markdown heading and before and after each fenced code block in CLAUDE.md (e.g.,
the "Before Committing", "Development Workflow", "Testing", "Dependency
Management", and "Claude Code Preferences" headings and theirbashfences)
so the file complies with MD022 and MD031; specifically, ensure there is an
empty line separating the heading from the previous paragraph and an empty line
between the fenced code opening/closing lines and surrounding text throughout
the file.</details> <!-- cr-comment:v1:fc5ea020d20ce7d1cb3e1afc --> </blockquote></details> <details> <summary>docs/api-contracts-guidelines.md (1)</summary><blockquote> `23-23`: _💤 Low value_ **Consider addressing markdown formatting for better consistency.** The markdownlint tool flagged several minor style issues: - Line 23: The code block should specify a language (e.g., ````go`) and be surrounded by blank lines - Several headings (lines 38, 43, 47, 54, 57, 82, 90, 95) should have blank lines before and after them These don't affect functionality but would improve markdown parsing consistency and potentially enable better syntax highlighting. Also applies to: 38-38, 43-43, 47-47, 54-54, 57-57, 82-82, 90-90, 95-95 <details> <summary>🤖 Prompt for AI Agents</summary>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.mdat line 23, Update the markdown for
docs/api-contracts-guidelines.md by adding a language specifier to the code
fence at the shown code block (e.g., changetogo) and ensure there is a
blank line before and after that fenced code block; also add a blank line before
and after each top-level heading referenced in the review (the headings at the
noted locations) so headings are separated from surrounding paragraphs and lists
for consistent markdownlint-compliant formatting.</details> <!-- cr-comment:v1:4182dd4ed530dcbd4d4b986e --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>docs/performance-guidelines.md (2)</summary><blockquote> `1-93`: _⚡ Quick win_ **Add blank lines below section headings for consistency.** Multiple level-3 headings (`###`) are missing blank lines below them before content begins (lines 5, 12, 19, 24, 38, 43, 51, 54, 59, 62, 65, 70, 77, 88, 91). Markdown best practice requires blank lines around headings for improved readability and consistency. <details> <summary>Example fix pattern</summary> ```diff ### Manager-Level Label-Filtered Cache + The operator uses a single manager-level cache... ``` Apply this pattern to all flagged headings. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/performance-guidelines.md` around lines 1 - 93, Add a single blank line after each level-3 Markdown heading that currently runs straight into content so headings follow the "blank line below headings" convention; specifically insert a blank line after the headings "### Manager-Level Label-Filtered Cache", "### Uncached Client Usage", "### Conditional CRD Cache Registration", "### Predicate Strategy by Resource Type", "### Map Function Convention", "### Error Classification and Requeue Strategy", "### Condition Update Optimization", "### CR Annotation Patching", "### UpdateWithRetry Pattern", "### Status Update Pattern", "### Bootstrap Resource Creation", "### Resettable sync.Once (`common.Now`)", "### HasObjectChanged Pattern", and "### Asset Decoding" in the document so each heading is followed by one empty line before its paragraph content. ``` </details> <!-- cr-comment:v1:e77c49c434d3ab410a8685d0 --> _Source: Linters/SAST tools_ --- `5-21`: _⚡ Quick win_ **Consider removing hard-coded line reference.** Line 8 references "line ~68" in `controller.go`. Hard-coded line numbers in documentation become stale as code evolves. Consider using a more resilient reference like "in `controllerManagedResources` constant" or "in the resource list near the cache builder initialization". <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/performance-guidelines.md` around lines 5 - 21, The doc references a hard-coded line number ("line ~68" in controller.go); replace that brittle reference with a resilient identifier such as the constant or symbol name (e.g., controllerManagedResources) and/or a contextual location (e.g., "the resource list near NewCacheBuilder() / buildCacheObjectList() in pkg/controller/external_secrets/controller.go") so readers can find where to register new resources without relying on line numbers. ``` </details> <!-- cr-comment:v1:ebe16097cce0cd6e4abe1069 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@AGENTS.md:
- Around line 28-49: The fenced directory-listing block in AGENTS.md is missing
a language identifier; update the opening triple-backticks that precede the
directory list (the block containing lines like "api/v1alpha1/ CRD
type definitions...") to include a plaintext identifier (e.g.,text) so the code fence renders and lints correctly; ensure only the first backtick fence is changed and the closing fence remainsunchanged.In
@CLAUDE.md:
- Line 3: Replace the nonstandard annotation '
@AGENTS.md' in CLAUDE.md with a
proper Markdown link or an explanatory comment; specifically locate the literal
'@AGENTS.md' token and either change it to a Markdown link like
AGENTS.md or, if it is tool-specific, add a short inline comment
explaining the syntax and its purpose so readers know why it isn't standard
Markdown.In
@docs/security-guidelines.md:
- Around line 51-55: The markdown fenced code block showing the regex for
disallowedLabelMatcher (in install_external_secrets.go) needs proper formatting:
add a blank line before and after the fence and include a language identifier
(e.g., "regex") on the opening fence so the block renders correctly; update the
docs/security-guidelines.md block accordingly around the regex snippet for
disallowedLabelMatcher.
Nitpick comments:
In@CLAUDE.md:
- Around line 9-51: Add a blank line before and after each Markdown heading and
before and after each fenced code block in CLAUDE.md (e.g., the "Before
Committing", "Development Workflow", "Testing", "Dependency Management", and
"Claude Code Preferences" headings and theirbashfences) so the file
complies with MD022 and MD031; specifically, ensure there is an empty line
separating the heading from the previous paragraph and an empty line between the
fenced code opening/closing lines and surrounding text throughout the file.In
@docs/api-contracts-guidelines.md:
- Line 23: Update the markdown for docs/api-contracts-guidelines.md by adding a
language specifier to the code fence at the shown code block (e.g., changetogo) and ensure there is a blank line before and after that fenced code
block; also add a blank line before and after each top-level heading referenced
in the review (the headings at the noted locations) so headings are separated
from surrounding paragraphs and lists for consistent markdownlint-compliant
formatting.In
@docs/performance-guidelines.md:
- Around line 1-93: Add a single blank line after each level-3 Markdown heading
that currently runs straight into content so headings follow the "blank line
below headings" convention; specifically insert a blank line after the headings
"### Manager-Level Label-Filtered Cache", "### Uncached Client Usage", "###
Conditional CRD Cache Registration", "### Predicate Strategy by Resource Type",
"### Map Function Convention", "### Error Classification and Requeue Strategy",
"### Condition Update Optimization", "### CR Annotation Patching", "###
UpdateWithRetry Pattern", "### Status Update Pattern", "### Bootstrap Resource
Creation", "### Resettable sync.Once (common.Now)", "### HasObjectChanged
Pattern", and "### Asset Decoding" in the document so each heading is followed
by one empty line before its paragraph content.- Around line 5-21: The doc references a hard-coded line number ("line ~68" in
controller.go); replace that brittle reference with a resilient identifier such
as the constant or symbol name (e.g., controllerManagedResources) and/or a
contextual location (e.g., "the resource list near NewCacheBuilder() /
buildCacheObjectList() in pkg/controller/external_secrets/controller.go") so
readers can find where to register new resources without relying on line
numbers.In
@docs/testing-guidelines.md:
- Around line 17-113: The Markdown file has multiple headings without
surrounding blank lines (violating MD022); fix by inserting a single blank line
before and after each heading token (e.g., the "### Conventions", "### Fake
Client Pattern", "### Test Helpers", "### Error Assertion", "### Adding a New
Unit Test", "## API Integration Tests (test/apis/)", "### Data-Driven Test
Suites", "### envtest Setup", "### Running", "### Adding a New API Test", "##
E2E Tests (test/e2e/)", "### Build Tag", "### Label-Based Platform Selection",
"### E2E Structure", "### Test Data", "### E2E Test Utilities (test/utils/)",
"### Adding a New E2E Test") so each heading is preceded and followed by exactly
one blank line to satisfy lint MD022 and improve readability.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Repository: openshift/coderabbit/.coderabbit.yaml **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `088f4b9d-3b92-48d9-a894-246e50996f77` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e9969c3b3dbb343ccdc9e781d9feffd549b74d71 and 13849aa8957958b3770a70ca98889d060d8ee63a. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `AGENTS.md` * `CLAUDE.md` * `docs/api-contracts-guidelines.md` * `docs/error-handling-guidelines.md` * `docs/integration-guidelines.md` * `docs/performance-guidelines.md` * `docs/security-guidelines.md` * `docs/testing-guidelines.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+28
to
+49
| ``` | ||
| api/v1alpha1/ CRD type definitions, conditions, shared types, deepcopy | ||
| api/v1alpha1/tests/ Declarative YAML test suites for CRD validation | ||
| bindata/ Static operand YAML manifests (compiled into Go via go-bindata) | ||
| cmd/ Operator entrypoint (main.go) | ||
| config/ Kustomize manifests (CRDs, RBAC, manager, samples, bundle) | ||
| hack/ Shell scripts (codegen, verification, CI helpers) | ||
| images/ci/ CI Dockerfiles (coverage-instrumented builds) | ||
| pkg/controller/ Controller implementations | ||
| client/ CtrlClient interface + counterfeiter fakes | ||
| common/ Shared utilities (errors, constants, decode helpers, drift detection) | ||
| commontest/ Shared test fixtures (TestExternalSecretsConfig, TestExternalSecretsManager) | ||
| crd_annotator/ CRD annotation controller | ||
| external_secrets/ Main operand reconciliation controller | ||
| external_secrets_manager/ Status aggregation controller | ||
| pkg/operator/ Manager setup, controller registration | ||
| pkg/version/ Build-time version info (ldflags) | ||
| test/apis/ API integration tests (Ginkgo + envtest) | ||
| test/e2e/ End-to-end tests (Ginkgo + live cluster) | ||
| test/utils/ E2E test helpers | ||
| tools/ Go module for build-time tool dependencies | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to the code fence.
The directory structure code block should specify a language identifier (e.g., text or plaintext) for proper rendering and linting compliance.
📝 Proposed fix
-```
+```text
api/v1alpha1/ CRD type definitions, conditions, shared types, deepcopy🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 28 - 49, The fenced directory-listing block in
AGENTS.md is missing a language identifier; update the opening triple-backticks
that precede the directory list (the block containing lines like "api/v1alpha1/
CRD type definitions...") to include a plaintext identifier (e.g., ```text) so
the code fence renders and lints correctly; ensure only the first backtick fence
is changed and the closing fence remains ``` unchanged.
| @@ -0,0 +1,51 @@ | |||
| # CLAUDE.md | |||
|
|
|||
| @AGENTS.md | |||
There was a problem hiding this comment.
Clarify or remove the @AGENTS.md annotation.
The @AGENTS.md syntax is not standard Markdown. If this is intended as a reference or link, use a proper Markdown link: [AGENTS.md](AGENTS.md). If it's meant for a specific tool or extension, add a comment explaining the syntax.
🔗 Proposed fix
# CLAUDE.md
-@AGENTS.md
+See [AGENTS.md](AGENTS.md) for comprehensive architectural details.📝 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
| @AGENTS.md | |
| # CLAUDE.md | |
| See [AGENTS.md](AGENTS.md) for comprehensive architectural details. |
🤖 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` at line 3, Replace the nonstandard annotation '`@AGENTS.md`' in
CLAUDE.md with a proper Markdown link or an explanatory comment; specifically
locate the literal '`@AGENTS.md`' token and either change it to a Markdown link
like [AGENTS.md](AGENTS.md) or, if it is tool-specific, add a short inline
comment explaining the syntax and its purpose so readers know why it isn't
standard Markdown.
Comment on lines
+51
to
+55
| The regex `disallowedLabelMatcher` in `install_external_secrets.go` prevents users from overriding internal labels via `ControllerConfig.Labels`: | ||
| ``` | ||
| ^app.kubernetes.io\/|^external-secrets.io\/|^rbac.authorization.k8s.io\/|^servicebinding.io\/controller$|^app$ | ||
| ``` | ||
| When adding new internal labels, add them to this regex. |
There was a problem hiding this comment.
Fix markdown formatting around the code fence.
The fenced code block needs blank lines before and after, and should specify a language identifier for proper rendering.
📝 Proposed fix
When adding new internal labels, add them to this regex.
+
-```
+```regex
^app.kubernetes.io\/|^external-secrets.io\/|^rbac.authorization.k8s.io\/|^servicebinding.io\/controller$|^app$When adding new internal labels, add them to this regex.
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 52-52: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
[warning] 52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
[warning] 54-54: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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 51 - 55, The markdown fenced code
block showing the regex for disallowedLabelMatcher (in
install_external_secrets.go) needs proper formatting: add a blank line before
and after the fence and include a language identifier (e.g., "regex") on the
opening fence so the block renders correctly; update the
docs/security-guidelines.md block accordingly around the regex snippet for
disallowedLabelMatcher.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- cr-comment:v1:f609374c70c42445ee95a6b7 -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
|
@siddhibhor-56: 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
1 participant
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