Skip to content

Commit ba715f8

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #2489 from JoelSpeed/agents
Add initial AI api-review configuration
2 parents 7837a80 + b0dd8fd commit ba715f8

File tree

3 files changed

+383
-0
lines changed

3 files changed

+383
-0
lines changed

‎.claude/commands/api-review.md‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
---
2+
name: api-review
3+
description: Run strict OpenShift API review workflow for PR changes or local changes
4+
parameters:
5+
- name: pr_url
6+
description: GitHub PR URL to review (optional - if not provided, reviews local changes against upstream master)
7+
required: false
8+
---
9+
10+
# Output Format Requirements
11+
You MUST use this EXACT format for ALL review feedback:
12+
13+
14+
+LineNumber: Brief description
15+
**Current (problematic) code:**
16+
```go
17+
[exact code from the PR diff]
18+
```
19+
20+
**Suggested change:**
21+
```diff
22+
- [old code line]
23+
+ [new code line]
24+
```
25+
26+
**Explanation:** [Why this change is needed]
27+
28+
29+
I'll run a comprehensive API review for OpenShift API changes. This can review either a specific GitHub PR or local changes against upstream master.
30+
31+
## Step 1: Pre-flight checks and determine review mode
32+
33+
First, I'll check the arguments and determine whether to review a PR or local changes:
34+
35+
```bash
36+
# Save current branch
37+
CURRENT_BRANCH=$(git branch --show-current)
38+
echo "📍 Current branch: $CURRENT_BRANCH"
39+
40+
# Check if a PR URL was provided
41+
if [ -n "$ARGUMENTS" ] && [[ "$ARGUMENTS" =~ github\.com.*pull ]]; then
42+
REVIEW_MODE="pr"
43+
PR_NUMBER=$(echo "$ARGUMENTS" | grep -oE '[0-9]+$')
44+
echo "🔍 PR review mode: Reviewing PR #$PR_NUMBER"
45+
46+
# For PR review, check for uncommitted changes
47+
if ! git diff --quiet || ! git diff --cached --quiet; then
48+
echo "❌ ERROR: Uncommitted changes detected. Cannot proceed with PR review."
49+
echo "Please commit or stash your changes before running the API review."
50+
git status --porcelain
51+
exit 1
52+
fi
53+
echo "✅ No uncommitted changes detected. Safe to proceed with PR review."
54+
else
55+
REVIEW_MODE="local"
56+
echo "🔍 Local review mode: Reviewing local changes against upstream master"
57+
58+
# Find a remote pointing to openshift/api repository
59+
OPENSHIFT_REMOTE=""
60+
for remote in $(git remote); do
61+
remote_url=$(git remote get-url "$remote" 2>/dev/null || echo "")
62+
if [[ "$remote_url" =~ github\.com[/:]openshift/api(\.git)?$ ]]; then
63+
OPENSHIFT_REMOTE="$remote"
64+
echo "✅ Found OpenShift API remote: '$remote' -> $remote_url"
65+
break
66+
fi
67+
done
68+
69+
# If no existing remote found, add upstream
70+
if [ -z "$OPENSHIFT_REMOTE" ]; then
71+
echo "⚠️ No remote pointing to openshift/api found. Adding upstream remote..."
72+
git remote add upstream https://github.com/openshift/api.git
73+
OPENSHIFT_REMOTE="upstream"
74+
fi
75+
76+
# Fetch latest changes from the OpenShift API remote
77+
echo "🔄 Fetching latest changes from $OPENSHIFT_REMOTE..."
78+
git fetch "$OPENSHIFT_REMOTE" master
79+
fi
80+
```
81+
82+
## Step 2: Get changed files based on review mode
83+
84+
```bash
85+
if [ "$REVIEW_MODE" = "pr" ]; then
86+
# PR Review: Checkout the PR and get changed files
87+
echo "🔄 Checking out PR #$PR_NUMBER..."
88+
gh pr checkout "$PR_NUMBER"
89+
90+
echo "📁 Analyzing changed files in PR..."
91+
CHANGED_FILES=$(gh pr view "$PR_NUMBER" --json files --jq '.files[].path' | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/')
92+
else
93+
# Local Review: Get changed files compared to openshift remote master
94+
echo "📁 Analyzing locally changed files compared to $OPENSHIFT_REMOTE/master..."
95+
CHANGED_FILES=$(git diff --name-only "$OPENSHIFT_REMOTE/master...HEAD" | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/')
96+
97+
# Also include staged changes
98+
STAGED_FILES=$(git diff --cached --name-only | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/' || true)
99+
if [ -n "$STAGED_FILES" ]; then
100+
CHANGED_FILES=$(echo -e "$CHANGED_FILES\n$STAGED_FILES" | sort -u)
101+
fi
102+
fi
103+
104+
echo "Changed API files:"
105+
echo "$CHANGED_FILES"
106+
107+
if [ -z "$CHANGED_FILES" ]; then
108+
echo "ℹ️ No API files changed. Nothing to review."
109+
if [ "$REVIEW_MODE" = "pr" ]; then
110+
git checkout "$CURRENT_BRANCH"
111+
fi
112+
exit 0
113+
fi
114+
```
115+
116+
## Step 3: Run linting checks on changes
117+
118+
```bash
119+
echo "⏳ Running linting checks on changes..."
120+
make lint
121+
122+
if [ $? -ne 0 ]; then
123+
echo "❌ Linting checks failed. Please fix the issues before proceeding."
124+
if [ "$REVIEW_MODE" = "pr" ]; then
125+
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
126+
git checkout "$CURRENT_BRANCH"
127+
fi
128+
exit 1
129+
fi
130+
131+
echo "✅ Linting checks passed."
132+
```
133+
134+
## Step 4: Documentation validation
135+
136+
For each changed API file, I'll validate:
137+
138+
1. **Field Documentation**: All struct fields must have documentation comments
139+
2. **Optional Field Behavior**: Optional fields must explain what happens when they are omitted
140+
3. **Validation Documentation**: Validation rules must be documented and match markers
141+
142+
Let me check each changed file for these requirements:
143+
144+
```thinking
145+
I need to analyze the changed files to:
146+
1. Find struct fields without documentation
147+
2. Find optional fields without behavior documentation
148+
3. Find validation annotations without corresponding documentation
149+
150+
For each Go file, I'll:
151+
- Look for struct field definitions
152+
- Check if they have preceding comment documentation
153+
- For optional fields (those with `+kubebuilder:validation:Optional` or `+optional`), verify behavior is explained
154+
- For fields with validation annotations, ensure the validation is documented
155+
```
156+
157+
## Step 5: Generate comprehensive review report
158+
159+
I'll provide a comprehensive report showing:
160+
- ✅ Files that pass all checks
161+
- ❌ Files with documentation issues
162+
- 📋 Specific lines that need attention
163+
- 📚 Guidance on fixing any issues
164+
165+
The review will fail if any documentation requirements are not met for the changed files.
166+
167+
## Step 6: Switch back to original branch (PR mode only)
168+
169+
After completing the review, if we were reviewing a PR, I'll switch back to the original branch:
170+
171+
```bash
172+
if [ "$REVIEW_MODE" = "pr" ]; then
173+
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
174+
git checkout "$CURRENT_BRANCH"
175+
echo "✅ API review complete. Back on branch: $(git branch --show-current)"
176+
else
177+
echo "✅ Local API review complete."
178+
fi
179+
```
180+
181+
**CRITICAL WORKFLOW REQUIREMENTS:**
182+
183+
**For PR Review Mode:**
184+
1. MUST check for uncommitted changes before starting
185+
2. MUST abort if uncommitted changes are detected
186+
3. MUST save current branch name before switching
187+
4. MUST checkout the PR before running `make lint`
188+
5. MUST switch back to original branch when complete
189+
6. If any step fails, MUST attempt to switch back to original branch before exiting
190+
191+
**For Local Review Mode:**
192+
1. MUST detect existing remotes pointing to openshift/api repository (supports any remote name)
193+
2. MUST add upstream remote only if no existing openshift/api remote is found
194+
3. MUST fetch latest changes from the detected openshift/api remote
195+
4. MUST compare against the detected remote's master branch
196+
5. MUST include both committed and staged changes in analysis
197+
6. No branch switching required since we're reviewing local changes

‎AGENTS.md‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
This file provides guidance to AI agents when working with code in this repository.
2+
3+
This is the OpenShift API repository - the canonical location of OpenShift API type definitions and serialization code. It contains:
4+
5+
- API type definitions for OpenShift-specific resources (Custom Resource Definitions)
6+
- FeatureGate management system for controlling API availability across cluster profiles
7+
- Generated CRD manifests and validation schemas
8+
- Integration test suite for API validation
9+
10+
## Key Architecture Components
11+
12+
### FeatureGate System
13+
The FeatureGate system (`features/features.go`) controls API availability across different cluster profiles (Hypershift, SelfManaged) and feature sets (Default, TechPreview, DevPreview). Each API feature is gated behind a FeatureGate that can be enabled/disabled per cluster profile and feature set.
14+
15+
### API Structure
16+
APIs are organized by group and version (e.g., `route/v1`, `config/v1`). Each API group contains:
17+
- `types.go` - Go type definitions
18+
- `zz_generated.*` files - Generated code (deepcopy, CRDs, etc.)
19+
- `tests/` directories - Integration test definitions
20+
- CRD manifest files
21+
22+
## Common Development Commands
23+
24+
### Building
25+
```bash
26+
make build # Build render and write-available-featuresets binaries
27+
make clean # Clean build artifacts
28+
```
29+
30+
### Code Generation
31+
```bash
32+
make update # Alias for update-codegen-crds
33+
```
34+
35+
### Testing
36+
```bash
37+
make test-unit # Run unit tests
38+
make integration # Run integration tests (in tests/ directory)
39+
go test -v ./... # Run tests for specific packages
40+
41+
# Run integration tests for specific API groups
42+
make -C config/v1 test # Run tests for config/v1 API group
43+
make -C route/v1 test # Run tests for route/v1 API group
44+
make -C operator/v1 test # Run tests for operator/v1 API group
45+
```
46+
47+
### Validation and Verification
48+
```bash
49+
make verify # Run all verification checks
50+
make verify-scripts # Verify generated code is up to date
51+
make verify-codegen-crds # Verify CRD generation is current
52+
make lint # Run golangci-lint (only on changes from master)
53+
make lint-fix # Auto-fix linting issues where possible
54+
```
55+
56+
## Adding New APIs
57+
58+
All APIs should start as tech preview.
59+
New fields on stable APIs should be introduced behind a feature gate `+openshift:enable:FeatureGate=MyFeatureGate`.
60+
61+
62+
### For New Stable APIs (v1)
63+
1. Create the API type with proper kubebuilder annotations
64+
2. Include required markers like `+openshift:compatibility-gen:level=1`
65+
3. Add validation tests in `<group>/<version>/tests/<crd-name>/`
66+
4. Run `make update-codegen-crds` to generate CRDs
67+
68+
### For New TechPreview APIs (v1alpha1)
69+
1. First add a FeatureGate in `features/features.go`
70+
2. Create the API type with `+openshift:enable:FeatureGate=MyFeatureGate`
71+
3. Add corresponding test files
72+
4. Run generation commands
73+
74+
### Adding FeatureGates
75+
Add to `features/features.go` using the builder pattern:
76+
```go
77+
FeatureGateMyFeatureName = newFeatureGate("MyFeatureName").
78+
reportProblemsToJiraComponent("my-jira-component").
79+
contactPerson("my-team-lead").
80+
productScope(ocpSpecific).
81+
enableIn(configv1.TechPreviewNoUpgrade).
82+
mustRegister()
83+
```
84+
85+
## Testing Framework
86+
87+
The repository includes a comprehensive integration test suite in `tests/`. Test suites are defined in `*.testsuite.yaml` files alongside API definitions and support:
88+
- `onCreate` tests for validation during resource creation
89+
- `onUpdate` tests for update-specific validations and immutability
90+
- Status subresource testing
91+
- Validation ratcheting tests using `initialCRDPatches`
92+
93+
Use `tests/hack/gen-minimal-test.sh $FOLDER $VERSION` to generate test suite templates.
94+
95+
## Container-based Development
96+
```bash
97+
make verify-with-container # Run verification in container
98+
make generate-with-container # Run code generation in container
99+
```
100+
101+
Uses `podman` by default, set `RUNTIME=docker` or `USE_DOCKER=1` to use Docker instead.
102+
103+
## Custom Claude Code Commands
104+
105+
### API Review
106+
```
107+
/api-review <pr-url>
108+
```
109+
Runs comprehensive API review for OpenShift API changes in a GitHub PR:
110+
- Executes `make lint` to check for kube-api-linter issues
111+
- Validates that all API fields are properly documented
112+
- Ensures optional fields explain behavior when not present
113+
- Confirms validation rules and kubebuilder markers are documented in field comments
114+
115+
#### Documentation Requirements
116+
All kubebuilder validation markers must be documented in the field's comment. For example:
117+
118+
**Good:**
119+
```go
120+
// internalDNSRecords is an optional field that determines whether we deploy
121+
// with internal records enabled for api, api-int, and ingress.
122+
// Valid values are "Enabled" and "Disabled".
123+
// When set to Enabled, in cluster DNS resolution will be enabled for the api, api-int, and ingress endpoints.
124+
// When set to Disabled, in cluster DNS resolution will be disabled and an external DNS solution must be provided for these endpoints.
125+
// +optional
126+
// +kubebuilder:validation:Enum=Enabled;Disabled
127+
InternalDNSRecords InternalDNSRecordsType `json:"internalDNSRecords"`
128+
```
129+
130+
**Bad:**
131+
```go
132+
// internalDNSRecords determines whether we deploy with internal records enabled for
133+
// api, api-int, and ingress.
134+
// +optional // ❌ Optional nature not documented in comment
135+
// +kubebuilder:validation:Enum=Enabled;Disabled // ❌ Valid values not documented
136+
InternalDNSRecords InternalDNSRecordsType `json:"internalDNSRecords"`
137+
```
138+
139+
#### Systematic Validation Marker Documentation Checklist
140+
141+
**MANDATORY**: For each field with validation markers, verify the comment documents ALL of the following that apply:
142+
143+
**Field Optionality:**
144+
- [ ] `+optional` - explain behavior when field is omitted
145+
- [ ] `+required` - explain that the field is required
146+
147+
**String/Array Length Constraints:**
148+
- [ ] `+kubebuilder:validation:MinLength` and `+kubebuilder:validation:MaxLength` - document character length constraints
149+
- [ ] `+kubebuilder:validation:MinItems` and `+kubebuilder:validation:MaxItems` - document item count ranges
150+
151+
**Value Constraints:**
152+
- [ ] `+kubebuilder:validation:Enum` - list all valid enum values and their meanings
153+
- [ ] `+kubebuilder:validation:Pattern` - explain the pattern requirement in human-readable terms
154+
- [ ] `+kubebuilder:validation:Minimum` and `+kubebuilder:validation:Maximum` - document numeric ranges
155+
156+
**Advanced Validation:**
157+
- [ ] `+kubebuilder:validation:XValidation` - explain cross-field validation rules in detail
158+
- [ ] Any custom validation logic - document the validation behavior
159+
160+
#### API Review Process
161+
162+
**CRITICAL PROCESS**: Follow this exact order to ensure comprehensive validation:
163+
164+
1. **Linting Check**: Run `make lint` and fix all kubeapilinter errors first
165+
2. **Extract Validation Markers**: Use systematic search to find all markers
166+
3. **Systematic Documentation Review**: For each marker found, verify corresponding documentation exists
167+
4. **Optional Fields Review**: Ensure every `+optional` field explains omitted behavior
168+
5. **Cross-field Validation**: Verify any documented field relationships have corresponding `XValidation` rules
169+
170+
**FAILURE CONDITIONS**: The review MUST fail if any of these are found:
171+
- Any validation marker without corresponding documentation
172+
- Any `+optional` field without omitted behavior explanation
173+
- Any documented field constraint without enforcement via validation rules
174+
- Any `make lint` failures
175+
176+
The comment must explicitly state:
177+
- When a field is optional (for `+kubebuilder:validation:Optional` or `+optional`)
178+
- Valid enum values (for `+kubebuilder:validation:Enum`)
179+
- Validation constraints (for min/max, patterns, etc.)
180+
- Default behavior when field is omitted
181+
- Any interactions with other fields, commonly implemented with `+kubebuilder:validation:XValidation`
182+
183+
**CRITICAL**: When API documentation states field relationships or constraints (e.g., "cannot be used together with field X", "mutually exclusive with field Y"), these relationships MUST be enforced with appropriate validation rules. Use `+kubebuilder:validation:XValidation` with CEL expressions for cross-field constraints. Documentation without enforcement is insufficient and will fail review.
184+
185+
Example: `/api-review https://github.com/openshift/api/pull/1234`

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
AGENTS.md

0 commit comments

Comments
 (0)