NO-JIRA: docs: improve AGENTS.md structure and accuracy#8297
NO-JIRA: docs: improve AGENTS.md structure and accuracy#8297openshift-merge-bot[bot] merged 5 commits intoopenshift:mainfrom
Conversation
Move API machinery fundamentals to api/AGENTS.md for better locality. Rename api/CLAUDE.md to api/AGENTS.md for consistent naming. Add pointers to design invariants and versioning docs. Fix Azure platform description and formatting inconsistencies. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
enxebre
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@enxebre: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR updates documentation: refactors root 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
![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 (1)
api/AGENTS.md (1)
50-50: Consider simplifying "outside of" to "outside".The phrase "outside of" can be simplified to just "outside" for conciseness, though the current phrasing is grammatically correct.
✨ Optional simplification
-This matters because consumers like ARO-HCP embed these types directly into their own structs and serialize them to storage (e.g., Cosmos DB) outside of CRD validation. If a consumer must revert to a previous code level, they need to deserialize data that was written by the newer version without errors or data corruption. +This matters because consumers like ARO-HCP embed these types directly into their own structs and serialize them to storage (e.g., Cosmos DB) outside CRD validation. If a consumer must revert to a previous code level, they need to deserialize data that was written by the newer version without errors or data corruption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/AGENTS.md` at line 50, Replace the phrase "outside of CRD validation" with the simplified "outside CRD validation" in the sentence that reads "This matters because consumers like ARO-HCP embed these types directly into their own structs and serialize them to storage (e.g., Cosmos DB) outside of CRD validation." Keep the rest of the sentence unchanged so it now reads "...serialize them to storage (e.g., Cosmos DB) outside CRD validation."
🤖 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`:
- Line 14: AGENTS.md references a non-existent file ".claude/skills.dev"; open
AGENTS.md, locate the ".claude/skills.dev" reference and either (a) create the
missing ".claude/skills.dev" file in the repo with the intended content, or (b)
update the reference in AGENTS.md to point to the correct existing file or
remove the broken link—ensure the change updates the link text and path
consistently so the document no longer points to a missing file.
In `@api/AGENTS.md`:
- Line 54: Replace the contradictory line about pointer types and `omitempty`
with a clear explanation: state that when changing a value type (e.g., `int32`)
to a pointer (`*int32`) you should add `omitempty` to avoid serializing `nil` as
`null`; explicitly note that adding `omitempty` makes the JSON field optional
(i.e., may be omitted) which is a semantic API change from required to optional;
and advise authors to evaluate whether converting a required field to an
optional one is acceptable for their use case. Reference the examples `int32` →
`*int32` and the `omitempty` tag so readers can locate and update the guidance
in AGENTS.md.
- Line 69: In AGENTS.md fix the duplicate word by replacing "see See
test/envtest/README.md" with "see test/envtest/README.md" so the sentence reads
correctly; update the text in the line containing the phrase to remove the extra
"See".
---
Nitpick comments:
In `@api/AGENTS.md`:
- Line 50: Replace the phrase "outside of CRD validation" with the simplified
"outside CRD validation" in the sentence that reads "This matters because
consumers like ARO-HCP embed these types directly into their own structs and
serialize them to storage (e.g., Cosmos DB) outside of CRD validation." Keep the
rest of the sentence unchanged so it now reads "...serialize them to storage
(e.g., Cosmos DB) outside CRD validation."
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e709ebb-c3f2-450c-9f3b-7b0ff5e1b34a
📒 Files selected for processing (3)
AGENTS.mdapi/AGENTS.mdapi/CLAUDE.md
💤 Files with no reviewable changes (1)
- api/CLAUDE.md
Fix skill path reference and remove duplicated "See" word. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/AGENTS.md (1)
54-54:⚠️ Potential issue | 🟡 MinorClarify the contradictory guidance on pointer types and
omitempty.The statement "Always pair pointer types with
omitemptyon required fields" is self-contradictory. A field withomitemptyis by definition optional in JSON (the key can be absent), while a "required field" must always be present. These are mutually exclusive.When changing a value type (e.g.,
int32) to a pointer (*int32), you must addomitemptyto preventnilfrom serializing asnull. However, addingomitemptychanges the field's semantics from required to optional in the JSON representation—a breaking API change that needs careful consideration.📝 Proposed clarification
-- **Changing a value type to a pointer** (e.g., `int32` to `*int32`): Without `omitempty`, a nil pointer serializes as `null`, which cannot be deserialized back into the non-pointer type. Always pair pointer types with `omitempty` on required fields. +- **Changing a value type to a pointer** (e.g., `int32` to `*int32`): Without `omitempty`, a nil pointer serializes as `null`, which cannot be deserialized back into the non-pointer type. You must add `omitempty` when introducing a pointer type, but note that this changes the field from required to optional in the JSON representation, which is a semantic API change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/AGENTS.md` at line 54, The wording about pointer types and omitempty is contradictory; update the guidance in the "Changing a value type to a pointer" section so it explains that adding `omitempty` prevents a nil pointer from serializing as `null` but also makes the JSON key optional (changing required semantics) and therefore is a breaking API change that must be reviewed. Replace the line "Always pair pointer types with `omitempty` on required fields" with a clear statement that when converting value fields (e.g., `int32`) to pointer types (e.g., `*int32`) you should add `omitempty` only if you accept the field becoming optional, and call out that this semantic change must be considered and communicated to API clients.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/AGENTS.md`:
- Line 54: The wording about pointer types and omitempty is contradictory;
update the guidance in the "Changing a value type to a pointer" section so it
explains that adding `omitempty` prevents a nil pointer from serializing as
`null` but also makes the JSON key optional (changing required semantics) and
therefore is a breaking API change that must be reviewed. Replace the line
"Always pair pointer types with `omitempty` on required fields" with a clear
statement that when converting value fields (e.g., `int32`) to pointer types
(e.g., `*int32`) you should add `omitempty` only if you accept the field
becoming optional, and call out that this semantic change must be considered and
communicated to API clients.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ac25bd12-6653-4b20-8d32-2f7c73256a7e
📒 Files selected for processing (2)
AGENTS.mdapi/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8297 +/- ##
=======================================
Coverage 36.02% 36.02%
=======================================
Files 767 767
Lines 93407 93407
=======================================
Hits 33649 33649
Misses 57053 57053
Partials 2705 2705 🚀 New features to boost your workflow:
|
Documenting the separation between control plane ingress (managed by HyperShift Private Link controllers) and guest cluster ingress (managed by the standard OpenShift ingress operator), including the purpose of .hypershift.local DNS records. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
openshift-ci
Bot
added
the
area/documentation
|
Now I have all the evidence. Here is the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Verify job runs Root CauseThe PR changed The Because the PR modified a source markdown file without running This is a straightforward "forgot to regenerate" error — the PR author needs to run Recommendations
Evidence
|
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
|
||
| For conventions read [https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md](https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md) | ||
|
|
||
| `make api-lint-fix` will enforce most conventions and best practices. |
There was a problem hiding this comment.
It might be worth adding an explicit "DO NOT FIGHT AGAINST ITS FINDINGS". I've had it try to ignore the suggestions several times in my AI travels with this make command.
| - APIs primarily in v1beta1 | ||
| - Any new API should GA as v1 | ||
| - Use feature gates for experimental functionality | ||
| - CRD generation via controller-gen with OpenShift-specific tooling |
There was a problem hiding this comment.
Should we add the make commands here?
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, enxebre 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 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aks
Failed TestsTotal failed tests: 3
e2e-aws
|
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/lgtm |
|
/verified bypass |
openshift-ci-robot
added
the
verified
|
@bryan-cox: The 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. |
|
/override "ci/prow/e2e-aks" Not needed for this PR |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/e2e-aks, ci/prow/e2e-azure-self-managed, ci/prow/e2e-kubevirt-aws-ovn-reduced 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 kubernetes-sigs/prow repository. |
|
/override "ci/prow/e2e-azure-self-managed" |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/e2e-azure-self-managed 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 kubernetes-sigs/prow repository. |
|
@enxebre: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Summary
api/AGENTS.mdfor better localityapi/CLAUDE.mdtoapi/AGENTS.mdfor consistent naming conventionTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit