-
Notifications
You must be signed in to change notification settings - Fork 573
Add initial AI api-review configuration #2489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,197 @@ | ||
--- | ||
name: api-review | ||
description: Run strict OpenShift API review workflow for PR changes or local changes | ||
parameters: | ||
- name: pr_url | ||
description: GitHub PR URL to review (optional - if not provided, reviews local changes against upstream master) | ||
required: false | ||
--- | ||
|
||
# Output Format Requirements | ||
You MUST use this EXACT format for ALL review feedback: | ||
|
||
|
||
+LineNumber: Brief description | ||
**Current (problematic) code:** | ||
```go | ||
[exact code from the PR diff] | ||
``` | ||
|
||
**Suggested change:** | ||
```diff | ||
- [old code line] | ||
+ [new code line] | ||
``` | ||
|
||
**Explanation:** [Why this change is needed] | ||
|
||
|
||
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. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to create a separate H1 heading to explain to the LLM that this is the steps for how to actually conduct the review? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weirdly, It's the sort of thing you want to build something to allow you to test :/ |
||
## Step 1: Pre-flight checks and determine review mode | ||
|
||
First, I'll check the arguments and determine whether to review a PR or local changes: | ||
|
||
```bash | ||
# Save current branch | ||
CURRENT_BRANCH=$(git branch --show-current) | ||
echo "📍 Current branch: $CURRENT_BRANCH" | ||
|
||
# Check if a PR URL was provided | ||
if [ -n "$ARGUMENTS" ] && [[ "$ARGUMENTS" =~ github\.com.*pull ]]; then | ||
REVIEW_MODE="pr" | ||
PR_NUMBER=$(echo "$ARGUMENTS" | grep -oE '[0-9]+$') | ||
echo "🔍 PR review mode: Reviewing PR #$PR_NUMBER" | ||
|
||
# For PR review, check for uncommitted changes | ||
if ! git diff --quiet || ! git diff --cached --quiet; then | ||
echo "❌ ERROR: Uncommitted changes detected. Cannot proceed with PR review." | ||
echo "Please commit or stash your changes before running the API review." | ||
git status --porcelain | ||
exit 1 | ||
fi | ||
echo "✅ No uncommitted changes detected. Safe to proceed with PR review." | ||
else | ||
REVIEW_MODE="local" | ||
echo "🔍 Local review mode: Reviewing local changes against upstream master" | ||
|
||
# Find a remote pointing to openshift/api repository | ||
OPENSHIFT_REMOTE="" | ||
for remote in $(git remote); do | ||
remote_url=$(git remote get-url "$remote" 2>/dev/null || echo "") | ||
if [[ "$remote_url" =~ github\.com[/:]openshift/api(\.git)?$ ]]; then | ||
OPENSHIFT_REMOTE="$remote" | ||
echo "✅ Found OpenShift API remote: '$remote' -> $remote_url" | ||
break | ||
fi | ||
done | ||
|
||
# If no existing remote found, add upstream | ||
if [ -z "$OPENSHIFT_REMOTE" ]; then | ||
echo "⚠️ No remote pointing to openshift/api found. Adding upstream remote..." | ||
git remote add upstream https://github.com/openshift/api.git | ||
OPENSHIFT_REMOTE="upstream" | ||
fi | ||
|
||
# Fetch latest changes from the OpenShift API remote | ||
echo "🔄 Fetching latest changes from $OPENSHIFT_REMOTE..." | ||
git fetch "$OPENSHIFT_REMOTE" master | ||
fi | ||
``` | ||
|
||
## Step 2: Get changed files based on review mode | ||
|
||
```bash | ||
if [ "$REVIEW_MODE" = "pr" ]; then | ||
# PR Review: Checkout the PR and get changed files | ||
echo "🔄 Checking out PR #$PR_NUMBER..." | ||
gh pr checkout "$PR_NUMBER" | ||
|
||
echo "📁 Analyzing changed files in PR..." | ||
CHANGED_FILES=$(gh pr view "$PR_NUMBER" --json files --jq '.files[].path' | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/') | ||
else | ||
# Local Review: Get changed files compared to openshift remote master | ||
echo "📁 Analyzing locally changed files compared to $OPENSHIFT_REMOTE/master..." | ||
CHANGED_FILES=$(git diff --name-only "$OPENSHIFT_REMOTE/master...HEAD" | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/') | ||
|
||
# Also include staged changes | ||
STAGED_FILES=$(git diff --cached --name-only | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/' || true) | ||
if [ -n "$STAGED_FILES" ]; then | ||
CHANGED_FILES=$(echo -e "$CHANGED_FILES\n$STAGED_FILES" | sort -u) | ||
fi | ||
fi | ||
|
||
echo "Changed API files:" | ||
echo "$CHANGED_FILES" | ||
|
||
if [ -z "$CHANGED_FILES" ]; then | ||
echo "ℹ️ No API files changed. Nothing to review." | ||
if [ "$REVIEW_MODE" = "pr" ]; then | ||
git checkout "$CURRENT_BRANCH" | ||
fi | ||
exit 0 | ||
fi | ||
``` | ||
|
||
## Step 3: Run linting checks on changes | ||
|
||
```bash | ||
echo "⏳ Running linting checks on changes..." | ||
make lint | ||
|
||
if [ $? -ne 0 ]; then | ||
echo "❌ Linting checks failed. Please fix the issues before proceeding." | ||
if [ "$REVIEW_MODE" = "pr" ]; then | ||
echo "🔄 Switching back to original branch: $CURRENT_BRANCH" | ||
git checkout "$CURRENT_BRANCH" | ||
fi | ||
exit 1 | ||
fi | ||
|
||
echo "✅ Linting checks passed." | ||
``` | ||
|
||
## Step 4: Documentation validation | ||
|
||
For each changed API file, I'll validate: | ||
|
||
1. **Field Documentation**: All struct fields must have documentation comments | ||
2. **Optional Field Behavior**: Optional fields must explain what happens when they are omitted | ||
3. **Validation Documentation**: Validation rules must be documented and match markers | ||
|
||
Let me check each changed file for these requirements: | ||
|
||
```thinking | ||
I need to analyze the changed files to: | ||
1. Find struct fields without documentation | ||
2. Find optional fields without behavior documentation | ||
3. Find validation annotations without corresponding documentation | ||
|
||
For each Go file, I'll: | ||
- Look for struct field definitions | ||
- Check if they have preceding comment documentation | ||
- For optional fields (those with `+kubebuilder:validation:Optional` or `+optional`), verify behavior is explained | ||
- For fields with validation annotations, ensure the validation is documented | ||
``` | ||
|
||
## Step 5: Generate comprehensive review report | ||
|
||
I'll provide a comprehensive report showing: | ||
- ✅ Files that pass all checks | ||
- ❌ Files with documentation issues | ||
- 📋 Specific lines that need attention | ||
- 📚 Guidance on fixing any issues | ||
|
||
The review will fail if any documentation requirements are not met for the changed files. | ||
|
||
## Step 6: Switch back to original branch (PR mode only) | ||
|
||
After completing the review, if we were reviewing a PR, I'll switch back to the original branch: | ||
|
||
```bash | ||
if [ "$REVIEW_MODE" = "pr" ]; then | ||
echo "🔄 Switching back to original branch: $CURRENT_BRANCH" | ||
git checkout "$CURRENT_BRANCH" | ||
echo "✅ API review complete. Back on branch: $(git branch --show-current)" | ||
else | ||
echo "✅ Local API review complete." | ||
fi | ||
``` | ||
|
||
**CRITICAL WORKFLOW REQUIREMENTS:** | ||
|
||
**For PR Review Mode:** | ||
1. MUST check for uncommitted changes before starting | ||
2. MUST abort if uncommitted changes are detected | ||
3. MUST save current branch name before switching | ||
4. MUST checkout the PR before running `make lint` | ||
5. MUST switch back to original branch when complete | ||
6. If any step fails, MUST attempt to switch back to original branch before exiting | ||
|
||
**For Local Review Mode:** | ||
1. MUST detect existing remotes pointing to openshift/api repository (supports any remote name) | ||
2. MUST add upstream remote only if no existing openshift/api remote is found | ||
3. MUST fetch latest changes from the detected openshift/api remote | ||
4. MUST compare against the detected remote's master branch | ||
5. MUST include both committed and staged changes in analysis | ||
6. No branch switching required since we're reviewing local changes |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,185 @@ | ||
This file provides guidance to AI agents when working with code in this repository. | ||
|
||
This is the OpenShift API repository - the canonical location of OpenShift API type definitions and serialization code. It contains: | ||
|
||
- API type definitions for OpenShift-specific resources (Custom Resource Definitions) | ||
- FeatureGate management system for controlling API availability across cluster profiles | ||
- Generated CRD manifests and validation schemas | ||
- Integration test suite for API validation | ||
|
||
## Key Architecture Components | ||
|
||
### FeatureGate System | ||
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. | ||
|
||
### API Structure | ||
APIs are organized by group and version (e.g., `route/v1`, `config/v1`). Each API group contains: | ||
- `types.go` - Go type definitions | ||
- `zz_generated.*` files - Generated code (deepcopy, CRDs, etc.) | ||
- `tests/` directories - Integration test definitions | ||
- CRD manifest files | ||
|
||
## Common Development Commands | ||
|
||
### Building | ||
```bash | ||
make build # Build render and write-available-featuresets binaries | ||
make clean # Clean build artifacts | ||
``` | ||
|
||
### Code Generation | ||
```bash | ||
make update # Alias for update-codegen-crds | ||
``` | ||
|
||
### Testing | ||
```bash | ||
make test-unit # Run unit tests | ||
make integration # Run integration tests (in tests/ directory) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we teach it how to run the integration tests with more focused arguments? That way running the integration tests don't take longer than necessary when reviewing a change that only impacts a subset of the APIs/tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's a good idea, I'll have a go at that |
||
go test -v ./... # Run tests for specific packages | ||
|
||
# Run integration tests for specific API groups | ||
make -C config/v1 test # Run tests for config/v1 API group | ||
make -C route/v1 test # Run tests for route/v1 API group | ||
make -C operator/v1 test # Run tests for operator/v1 API group | ||
``` | ||
|
||
### Validation and Verification | ||
```bash | ||
make verify # Run all verification checks | ||
make verify-scripts # Verify generated code is up to date | ||
make verify-codegen-crds # Verify CRD generation is current | ||
make lint # Run golangci-lint (only on changes from master) | ||
make lint-fix # Auto-fix linting issues where possible | ||
``` | ||
|
||
## Adding New APIs | ||
|
||
All APIs should start as tech preview. | ||
New fields on stable APIs should be introduced behind a feature gate `+openshift:enable:FeatureGate=MyFeatureGate`. | ||
|
||
|
||
### For New Stable APIs (v1) | ||
1. Create the API type with proper kubebuilder annotations | ||
2. Include required markers like `+openshift:compatibility-gen:level=1` | ||
3. Add validation tests in `<group>/<version>/tests/<crd-name>/` | ||
4. Run `make update-codegen-crds` to generate CRDs | ||
|
||
### For New TechPreview APIs (v1alpha1) | ||
1. First add a FeatureGate in `features/features.go` | ||
2. Create the API type with `+openshift:enable:FeatureGate=MyFeatureGate` | ||
3. Add corresponding test files | ||
4. Run generation commands | ||
|
||
### Adding FeatureGates | ||
Add to `features/features.go` using the builder pattern: | ||
```go | ||
FeatureGateMyFeatureName = newFeatureGate("MyFeatureName"). | ||
reportProblemsToJiraComponent("my-jira-component"). | ||
contactPerson("my-team-lead"). | ||
productScope(ocpSpecific). | ||
enableIn(configv1.TechPreviewNoUpgrade). | ||
mustRegister() | ||
``` | ||
|
||
## Testing Framework | ||
|
||
The repository includes a comprehensive integration test suite in `tests/`. Test suites are defined in `*.testsuite.yaml` files alongside API definitions and support: | ||
- `onCreate` tests for validation during resource creation | ||
- `onUpdate` tests for update-specific validations and immutability | ||
- Status subresource testing | ||
- Validation ratcheting tests using `initialCRDPatches` | ||
|
||
Use `tests/hack/gen-minimal-test.sh $FOLDER $VERSION` to generate test suite templates. | ||
|
||
## Container-based Development | ||
```bash | ||
make verify-with-container # Run verification in container | ||
make generate-with-container # Run code generation in container | ||
``` | ||
|
||
Uses `podman` by default, set `RUNTIME=docker` or `USE_DOCKER=1` to use Docker instead. | ||
|
||
## Custom Claude Code Commands | ||
|
||
### API Review | ||
``` | ||
/api-review <pr-url> | ||
``` | ||
Runs comprehensive API review for OpenShift API changes in a GitHub PR: | ||
- Executes `make lint` to check for kube-api-linter issues | ||
- Validates that all API fields are properly documented | ||
- Ensures optional fields explain behavior when not present | ||
- Confirms validation rules and kubebuilder markers are documented in field comments | ||
|
||
#### Documentation Requirements | ||
All kubebuilder validation markers must be documented in the field's comment. For example: | ||
|
||
**Good:** | ||
```go | ||
// internalDNSRecords is an optional field that determines whether we deploy | ||
// with internal records enabled for api, api-int, and ingress. | ||
// Valid values are "Enabled" and "Disabled". | ||
// When set to Enabled, in cluster DNS resolution will be enabled for the api, api-int, and ingress endpoints. | ||
// When set to Disabled, in cluster DNS resolution will be disabled and an external DNS solution must be provided for these endpoints. | ||
// +optional | ||
// +kubebuilder:validation:Enum=Enabled;Disabled | ||
InternalDNSRecords InternalDNSRecordsType `json:"internalDNSRecords"` | ||
``` | ||
|
||
**Bad:** | ||
```go | ||
// internalDNSRecords determines whether we deploy with internal records enabled for | ||
// api, api-int, and ingress. | ||
// +optional // ❌ Optional nature not documented in comment | ||
// +kubebuilder:validation:Enum=Enabled;Disabled // ❌ Valid values not documented | ||
InternalDNSRecords InternalDNSRecordsType `json:"internalDNSRecords"` | ||
``` | ||
|
||
#### Systematic Validation Marker Documentation Checklist | ||
|
||
**MANDATORY**: For each field with validation markers, verify the comment documents ALL of the following that apply: | ||
|
||
**Field Optionality:** | ||
- [ ] `+optional` - explain behavior when field is omitted | ||
- [ ] `+required` - explain that the field is required | ||
|
||
**String/Array Length Constraints:** | ||
- [ ] `+kubebuilder:validation:MinLength` and `+kubebuilder:validation:MaxLength` - document character length constraints | ||
- [ ] `+kubebuilder:validation:MinItems` and `+kubebuilder:validation:MaxItems` - document item count ranges | ||
|
||
**Value Constraints:** | ||
- [ ] `+kubebuilder:validation:Enum` - list all valid enum values and their meanings | ||
- [ ] `+kubebuilder:validation:Pattern` - explain the pattern requirement in human-readable terms | ||
- [ ] `+kubebuilder:validation:Minimum` and `+kubebuilder:validation:Maximum` - document numeric ranges | ||
|
||
**Advanced Validation:** | ||
- [ ] `+kubebuilder:validation:XValidation` - explain cross-field validation rules in detail | ||
- [ ] Any custom validation logic - document the validation behavior | ||
|
||
#### API Review Process | ||
|
||
**CRITICAL PROCESS**: Follow this exact order to ensure comprehensive validation: | ||
|
||
1. **Linting Check**: Run `make lint` and fix all kubeapilinter errors first | ||
2. **Extract Validation Markers**: Use systematic search to find all markers | ||
3. **Systematic Documentation Review**: For each marker found, verify corresponding documentation exists | ||
4. **Optional Fields Review**: Ensure every `+optional` field explains omitted behavior | ||
5. **Cross-field Validation**: Verify any documented field relationships have corresponding `XValidation` rules | ||
|
||
**FAILURE CONDITIONS**: The review MUST fail if any of these are found: | ||
- Any validation marker without corresponding documentation | ||
- Any `+optional` field without omitted behavior explanation | ||
- Any documented field constraint without enforcement via validation rules | ||
- Any `make lint` failures | ||
|
||
The comment must explicitly state: | ||
- When a field is optional (for `+kubebuilder:validation:Optional` or `+optional`) | ||
- Valid enum values (for `+kubebuilder:validation:Enum`) | ||
- Validation constraints (for min/max, patterns, etc.) | ||
- Default behavior when field is omitted | ||
- Any interactions with other fields, commonly implemented with `+kubebuilder:validation:XValidation` | ||
|
||
**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. | ||
|
||
Example: `/api-review https://github.com/openshift/api/pull/1234` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
AGENTS.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this parameters block coming from? I can only see $N args convention in claude docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude generated most of this document itself based on guidance we were giving it through the CLI, I'm pretty certain it wrote this out
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://docs.claude.com/en/docs/claude-code/slash-commands#parameters
We're just naming, and explicitly requiring them here, I think..the docs aren't too clear