NE-2390: Adding AGENTS.md file#1341
NE-2390: Adding AGENTS.md file#1341openshift-merge-bot[bot] merged 3 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@Thealisyed: This pull request references NE-2390 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.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. |
Thealisyed
force-pushed
the
addingagentmd
branch
from
7c899b5 to
285abcf
Compare
Adding this AGENTS.md file will allow all ai agents to index and refer to this file for contextual awareness Assisted with Claude
Thealisyed
force-pushed
the
addingagentmd
branch
from
285abcf to
0e2bf51
Compare
|
All team members should check into this, with Brett as lead. /assign @bentito |
bentito
left a comment
bentito
left a comment
There was a problem hiding this comment.
Overall, this is a strong addition that will significantly help agents (and humans) navigate the repo.
Review Comments
Strengths:
- Naming Conventions: The table of naming helpers (like RouterDeploymentName) is excellent. This is the #1 thing agents get wrong (hallucinating names), so having the canonical list is huge.
- Structure: The directory tree provides a good mental map.
Suggestions for Improvement:
-
Refine "Project Structure":
- The entry
pkg/operator/controller/ # Reconciliation controllersimplies the controllers are in that directory. In reality, they are in subdirectories (e.g.,pkg/operator/controller/ingress/). - Suggestion: Clarify that this directory contains sub-packages for each controller type.
- The entry
-
Document the
ensurePattern:- The codebase heavily uses an
ensureXpattern in reconcilers (e.g.,ensureIngressController,ensureIngressDeletedinpkg/operator/controller/ingress/controller.go). - Suggestion: Add a note in the "Coding Style" or "Architecture" section: "Controllers typically delegate logic to
ensure<Resource>methods that handle creation/update of specific resources."
- The codebase heavily uses an
-
Encourage Constants over Strings:
- The "Important Annotations" section lists raw string values.
- Suggestion: Reference the Go constants (e.g.,
controller.IngressOperatorOwnedAnnotationinpkg/operator/controller/names.go). This encourages agents to import the constant rather than hardcoding the string, which is safer.
-
Reference a "Golden" Test File:
- Suggestion: Explicitly point to
pkg/operator/controller/ingress/controller_test.goas the standard for how controller tests should be written. This gives an agent a concrete template to mimic.
- Suggestion: Explicitly point to
-
Manifests:
- A brief mention of
pkg/manifestsand how assets are loaded/bound would be helpful context, as valid manifest generation is often tricky.
- A brief mention of
Great work on this doc!
Miciah
left a comment
Miciah
left a comment
There was a problem hiding this comment.
It is interesting to compare this with openshift/router#710. In particular, is it useful to mention conventions for commit messages, PR descriptions, test coverage, and logical commits?
There is also some overlap with the design details in #1339, but that has a lot of content that was AI-generated and needs to be verified.
| cluster-ingress-operator/ | ||
| ├── cmd/ingress-operator/ # Main entry point | ||
| │ ├── main.go # CLI commands (start, render) | ||
| │ └── start.go # Operator initialization and controller registration |
There was a problem hiding this comment.
Controllers are registered in pkg/operator/operator.go.
There was a problem hiding this comment.
This is addressed in commit 76aebbe. The AGENTS.md file has been updated to correctly identify pkg/operator/operator.go for controller registration and cmd/ingress-operator/start.go for metrics.
|
|
||
| ### Error Handling | ||
|
|
||
| - Return errors with context: `fmt.Errorf("failed to create deployment: %w", err)` |
There was a problem hiding this comment.
Worth calling out explicitly: Use %w for wrapped error values (as in the example).
There was a problem hiding this comment.
This is addressed in commit 76aebbe. Explicit instructions to use %w for error wrapping and to follow Kubernetes logging conventions have been added to the 'Coding Style' section.
| ### Logging | ||
|
|
||
| - Use structured logging via `go-logr/logr` | ||
| - Include relevant context (namespace, name, resource type) |
There was a problem hiding this comment.
commit 76aebbe added instructions to follow Kubernetes logging conventions to the 'Coding Style' section.
Assisted with Claude
WalkthroughAdds a new repository-wide documentation file Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@Thealisyed: This pull request references NE-2390 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 7-32: The fenced code block in AGENTS.md that contains the
repository tree snippet is missing a language identifier; update the opening
triple backticks for that block to "```text" so the snippet is treated as plain
text (i.e., change the block starting with "```" before the
cluster-ingress-operator/ tree to "```text").
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
AGENTS.md
| ``` | ||
| cluster-ingress-operator/ | ||
| ├── cmd/ingress-operator/ # Main entry point | ||
| │ ├── main.go # CLI commands (start, render) | ||
| │ ├── render.go # Manifest rendering command | ||
| │ └── start.go # Operator startup and metrics registration | ||
| ├── pkg/ | ||
| │ ├── operator/ | ||
| │ │ ├── operator.go # Controller registration | ||
| │ │ ├── controller/ # Reconciliation controllers (sub-packages per controller) | ||
| │ │ ├── client/ # Kubernetes client setup with custom schemes | ||
| │ │ └── config/ # Operator configuration structure | ||
| │ ├── dns/ # DNS provider implementations | ||
| │ │ ├── aws/ # AWS Route 53 | ||
| │ │ ├── azure/ # Azure DNS | ||
| │ │ ├── gcp/ # Google Cloud DNS | ||
| │ │ ├── ibm/ # IBM Cloud DNS (public and private) | ||
| │ │ └── split/ # Meta-provider routing between public/private | ||
| │ └── manifests/ # Kubernetes object manifests used by controllers | ||
| ├── manifests/ # CVO manifests (CRDs, RBAC, monitoring) — instantiated by CVO, not used by operator directly | ||
| ├── test/ | ||
| │ └── e2e/ # End-to-end integration tests | ||
| ├── hack/ # Development and CI scripts | ||
| ├── Makefile # Build automation | ||
| └── HACKING.md # Developer documentation | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced block.
Line 7 uses a fenced code block without a language, which triggers MD040. Use text for the tree snippet.
Suggested diff
-```
+```text
cluster-ingress-operator/
├── cmd/ingress-operator/ # Main entry point
│ ├── main.go # CLI commands (start, render)
│ ├── render.go # Manifest rendering command
│ └── start.go # Operator startup and metrics registration
...
└── HACKING.md # Developer documentation</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @AGENTS.md around lines 7 - 32, The fenced code block in AGENTS.md that
contains the repository tree snippet is missing a language identifier; update
the opening triple backticks for that block to "text" so the snippet is treated as plain text (i.e., change the block starting with "" before the
cluster-ingress-operator/ tree to "```text").
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
Sorry, this review is much delayed, but for now just a top-level comment that will require an added chunk: You've got a good code standards outine so far and decent repo layout to quick orientation, but I think it needs more of an overall this is how CIO fits in: The AGENTS.md needs to orient the agent on CIO's place in the cluster networking galaxy. I'm not sure about the ideal set of quick knowledge we can convey, but here's a starter list:
We should really pull together several people for a meeting with the agenda titled "What would you tell a new coder about CIO in their first month on the team?", digest the notes from that meeting and put it in AGENTS.md Here's an example proposal, something we could discuss at a meeting to expand, delete, change: AGENTS.md1. SYSTEM CONTEXT & TOPOLOGY
2. STRICT NAMESPACE BOUNDARIES
3. CUSTOM RESOURCE DEFINITION (CRD) MATRIXCore Configuration
Gateway API (GWAPI)
4. DIRECTORY TOPOGRAPHY
5. TACTICAL DIRECTIVES & CONSTRAINTS
6. EXPLICIT NON-GOALS
|
|
Yes I agree @bentito. That would be the best approach not only for CIO but also CDO too. Having all our SME's discuss to ensure we capture to the best of our ability for the AGENTS.md file in each respective repo |
AGENTS.md is a good choice because Claude respects it in the same way as the proprietary CLAUDE.md |
|
@Thealisyed: This pull request references NE-2390 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
AGENTS.md (1)
50-75:⚠️ Potential issue | 🟡 MinorAdd language identifier to the fenced code block.
The code block displaying the project directory tree is missing a language identifier, triggering the MD040 markdown linting warning.
📝 Proposed fix
-``` +```text cluster-ingress-operator/ ├── cmd/ingress-operator/ # Main entry point🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 50 - 75, The fenced code block showing the project directory tree is missing a language identifier; update the opening fence from ``` to ```text so the block is explicitly marked as plain text (modify the fenced block that contains the directory tree starting with "cluster-ingress-operator/" in AGENTS.md).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@AGENTS.md`:
- Around line 50-75: The fenced code block showing the project directory tree is
missing a language identifier; update the opening fence from ``` to ```text so
the block is explicitly marked as plain text (modify the fenced block that
contains the directory tree starting with "cluster-ingress-operator/" in
AGENTS.md).
|
I've added to the doc along the lines of my comment about needing to help orient an agent on CIO's place in the NIDS world. Comments on whether that orientation seems correct and complete are very welcome. |
|
@Thealisyed: 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. |
|
/lgtm I'd like to merge this and then have a team discussion about what's needed more broadly for agents to do well (better?) on all of our repos and move to a scenario where AGENTS.md is just a table of contents into a |
|
/verified later @mjoseph |
openshift-ci-robot
added
the
verified
|
@melvinjoseph86: This PR has been marked to be verified later by 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidesalerno 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
openshift-merge-bot
Bot
merged commit 1aec6b8
into
openshift:master
7 participants
Adding AGENTS.md file
Adding this AGENTS.md file will allow
all ai agents to index and refer to
this file for contextual awareness