WIP: 🌱 Add CLAUDE command and new check to review and lint the API changes #2399
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
camilamacedo86
force-pushed
the
command-ai
branch
from
22a7443 to
5e3eb41
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new API linting workflow that validates Kubernetes API changes using kube-api-linter with diff-aware analysis. The implementation distinguishes between newly introduced issues and pre-existing problems, helping developers focus on fixing only the issues they've introduced.
Key Changes:
- Adds a shell script that builds a custom golangci-lint with kube-api-linter plugin and performs diff-based analysis
- Provides Claude AI integration with detailed instructions for intelligent issue analysis and false positive filtering
- Implements git worktree-based comparison between current branch and main branch
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
hack/api-lint-diff/run.sh |
Shell script that orchestrates the linting workflow: builds custom golangci-lint, runs linter on current and baseline branches, categorizes issues as NEW or PRE-EXISTING, and generates a report |
.claude/commands/api-lint-diff.md |
Documentation for Claude AI providing instructions for executing the script, filtering false positives, analyzing code usage patterns, and generating comprehensive reports |
Comments suppressed due to low confidence (2)
.claude/commands/api-lint-diff.md:191
- The documentation states that false positives are "silently ignored" and filtered out, but the shell script doesn't actually implement any false positive filtering logic. The script only runs the linter and categorizes issues as NEW or PRE-EXISTING. The false positive filtering is expected to be done by Claude AI when processing the script's output, not by the script itself. This creates a discrepancy between what the documentation says the command does versus what the script actually does. Consider either implementing the filtering in the script or clarifying in the documentation that the filtering is performed by AI analysis, not by the script.
.claude/commands/api-lint-diff.md:252 - This section states that false positives are "silently ignored" and "No false positives shown in output", but the actual script implementation (run.sh) does not perform any false positive filtering. All issues detected by the linter are shown in the output and categorized only as NEW or PRE-EXISTING. The false positive filtering is documented to be performed by Claude AI after the script runs, not by the script itself. This mismatch could confuse users who expect the script output to already have false positives filtered out.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while IFS= read -r line; do | ||
| [[ -z "${line}" ]] && continue | ||
|
|
||
| local file | ||
| file=$(echo "${line}" | cut -d: -f1) | ||
|
|
||
| # Check if file was changed | ||
| local file_changed=false | ||
| for changed_file in "${changed_files[@]}"; do | ||
| if [[ "${file}" == "${changed_file}" ]]; then | ||
| file_changed=true | ||
| break | ||
| fi | ||
| done | ||
|
|
||
| # If file wasn't changed, it's pre-existing | ||
| if [[ "${file_changed}" == "false" ]]; then | ||
| echo "${line}" >> "${preexisting_issues_file}" | ||
| continue | ||
| fi | ||
|
|
||
| # Check if issue exists in baseline | ||
| # Compare without line numbers since line numbers can change when code is added/removed | ||
| # Format is: file:line:col:linter:message | ||
| # We'll compare: file:linter:message | ||
| local file_linter_msg | ||
| file_linter_msg=$(echo "${line}" | cut -d: -f1,4,5) | ||
|
|
||
| if grep -F ":$(echo "${line}" | cut -d: -f4,5)" "${baseline_file}" 2>/dev/null | \ | ||
| grep -Fq "$(echo "${line}" | cut -d: -f1):"; then | ||
| echo "${line}" >> "${preexisting_issues_file}" | ||
| else | ||
| echo "${line}" >> "${new_issues_file}" | ||
| fi | ||
| done < "${current_file}" |
There was a problem hiding this comment.
If parse_linter_output produces no output (e.g., no issues found or grep returns no matches), the current_parsed.txt file will be empty. The while loop that reads from this file will handle this correctly by not executing the loop body, but it would be more defensive to ensure the file exists before attempting to read from it. Consider adding a check like 'if [[ -f "${current_file}" && -s "${current_file}" ]]; then' before the while loop to skip processing if the file doesn't exist or is empty.
| while IFS= read -r line; do | |
| [[ -z "${line}" ]] && continue | |
| local file | |
| file=$(echo "${line}" | cut -d: -f1) | |
| # Check if file was changed | |
| local file_changed=false | |
| for changed_file in "${changed_files[@]}"; do | |
| if [[ "${file}" == "${changed_file}" ]]; then | |
| file_changed=true | |
| break | |
| fi | |
| done | |
| # If file wasn't changed, it's pre-existing | |
| if [[ "${file_changed}" == "false" ]]; then | |
| echo "${line}" >> "${preexisting_issues_file}" | |
| continue | |
| fi | |
| # Check if issue exists in baseline | |
| # Compare without line numbers since line numbers can change when code is added/removed | |
| # Format is: file:line:col:linter:message | |
| # We'll compare: file:linter:message | |
| local file_linter_msg | |
| file_linter_msg=$(echo "${line}" | cut -d: -f1,4,5) | |
| if grep -F ":$(echo "${line}" | cut -d: -f4,5)" "${baseline_file}" 2>/dev/null | \ | |
| grep -Fq "$(echo "${line}" | cut -d: -f1):"; then | |
| echo "${line}" >> "${preexisting_issues_file}" | |
| else | |
| echo "${line}" >> "${new_issues_file}" | |
| fi | |
| done < "${current_file}" | |
| if [[ -f "${current_file}" && -s "${current_file}" ]]; then | |
| while IFS= read -r line; do | |
| [[ -z "${line}" ]] && continue | |
| local file | |
| file=$(echo "${line}" | cut -d: -f1) | |
| # Check if file was changed | |
| local file_changed=false | |
| for changed_file in "${changed_files[@]}"; do | |
| if [[ "${file}" == "${changed_file}" ]]; then | |
| file_changed=true | |
| break | |
| fi | |
| done | |
| # If file wasn't changed, it's pre-existing | |
| if [[ "${file_changed}" == "false" ]]; then | |
| echo "${line}" >> "${preexisting_issues_file}" | |
| continue | |
| fi | |
| # Check if issue exists in baseline | |
| # Compare without line numbers since line numbers can change when code is added/removed | |
| # Format is: file:line:col:linter:message | |
| # We'll compare: file:linter:message | |
| local file_linter_msg | |
| file_linter_msg=$(echo "${line}" | cut -d: -f1,4,5) | |
| if grep -F ":$(echo "${line}" | cut -d: -f4,5)" "${baseline_file}" 2>/dev/null | \ | |
| grep -Fq "$(echo "${line}" | cut -d: -f1):"; then | |
| echo "${line}" >> "${preexisting_issues_file}" | |
| else | |
| echo "${line}" >> "${new_issues_file}" | |
| fi | |
| done < "${current_file}" | |
| fi |
| # Get golangci-lint version from bingo | ||
| local golangci_version | ||
| golangci_version=$(get_golangci_version) | ||
|
|
There was a problem hiding this comment.
The golangci_version variable could be empty if get_golangci_version fails to extract the version from Variables.mk. This would result in an invalid config file with "version: " (empty value). Consider adding validation to ensure the version is not empty before using it in the config, or handle the error case explicitly.
| if [[ -z "${golangci_version}" ]]; then | |
| echo -e "${RED}Error: Failed to determine golangci-lint version from .bingo/Variables.mk${NC}" >&2 | |
| exit 1 | |
| fi |
| if grep -F ":$(echo "${line}" | cut -d: -f4,5)" "${baseline_file}" 2>/dev/null | \ | ||
| grep -Fq "$(echo "${line}" | cut -d: -f1):"; then |
There was a problem hiding this comment.
The variable file_linter_msg is assigned on line 279 but never used. The comparison logic on lines 281-282 directly uses cut commands without referencing this variable. Either use this variable in the comparison or remove the assignment to improve code clarity and maintainability.
| if grep -F ":$(echo "${line}" | cut -d: -f4,5)" "${baseline_file}" 2>/dev/null | \ | |
| grep -Fq "$(echo "${line}" | cut -d: -f1):"; then | |
| if grep -Fq "${file_linter_msg}" "${baseline_file}" 2>/dev/null; then |
camilamacedo86
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
command-ai
branch
from
5e3eb41 to
a625380
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2399 +/- ##
==========================================
+ Coverage 68.82% 73.05% +4.22%
==========================================
Files 100 100
Lines 7641 7641
==========================================
+ Hits 5259 5582 +323
+ Misses 1947 1623 -324
- Partials 435 436 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
What This Command Does
api/mainto identify:optionalfieldson Status field (Status is never a pointer in K8s)requiredfieldson Spec field (Spec is never a pointer in K8s)conditionson[]metav1.Condition(patch semantics already correct)More info: CRD scaffolds trigger false positives with kube-api-linter for Operator Projects kubernetes-sigs/kube-api-linter#192
Why it matters
EXAMPLE - Result