NO-JIRA: Add API dependency import restriction tool - #8364
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@JoelSpeed: 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a Go-based API dependency verifier and integrates it into the Makefile. The Makefile defines Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as Developer/CI
participant Make as Makefile
participant Build as Go Tool Builder
participant Tool as verify-api-deps
participant FS as FileSystem
Runner->>Make: run `make verify` / `make verify-parallel`
Make->>Build: build $(VERIFY_API_DEPS)
Build-->>Make: built binary
Make->>Tool: execute verify-api-deps
Tool->>FS: read "api/.imports_allowed"
Tool->>FS: read "api/go.mod"
Tool->>Tool: parse go.mod, filter direct requires
Tool->>Tool: compare requires against allowlist
alt violations found
Tool-->>Make: exit 1 + violation report (stderr)
Make-->>Runner: verification failed
else no violations
Tool-->>Make: exit 0 + success message (stdout)
Make-->>Runner: verification passed
end
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
| // allowedAPIModules defines the restricted list of allowed direct dependencies for the API module. | ||
| // Any new dependencies MUST be reviewed by API reviewers BEFORE being added to this list. | ||
| // Note: Indirect dependencies are automatically ignored by the verification logic. | ||
| var allowedAPIModules = sets.New( | ||
| // Core Kubernetes API dependencies | ||
| "k8s.io/api", | ||
| "k8s.io/apimachinery", | ||
| "k8s.io/utils", | ||
|
|
||
| // OpenShift API dependencies | ||
| "github.com/openshift/api", | ||
| ) |
There was a problem hiding this comment.
Possibly this would be better as a file in the api folder so that it comes under the API OWNERS file rather than the root OWNERS file
openshift-ci
Bot
added
area/ci-tooling
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8364 +/- ##
=======================================
Coverage 36.46% 36.46%
=======================================
Files 765 765
Lines 93256 93256
=======================================
Hits 34010 34010
Misses 56532 56532
Partials 2714 2714
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 `@hack/tools/verify-api-deps/README.md`:
- Around line 29-31: The README currently tells users to change into
hack/tools/verify-api-deps then run the tool, which makes Go look for api/go.mod
in that subdirectory and fails; update the instructions in README.md for the
verify-api-deps tool to tell users to run the tool from the repository root and
invoke go run against hack/tools/verify-api-deps/main.go (i.e., do not cd into
the subdirectory), so the correct api/go.mod is resolved when executing main.go.
🪄 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: Enterprise
Run ID: bd4a9e96-baa9-4570-a68d-8e7f9e796c7a
📒 Files selected for processing (3)
Makefilehack/tools/verify-api-deps/README.mdhack/tools/verify-api-deps/main.go
openshift-ci
Bot
added
the
area/api
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 `@hack/tools/verify-api-deps/main.go`:
- Around line 60-82: The error string returned when len(violations) > 0
incorrectly instructs users to "Update the allowlist in
hack/tools/verify-api-deps/main.go"; update the fmt.Errorf message (the return
fmt.Errorf(...) that uses formatViolations(violations)) to remove the stale
reference and instead direct users to the canonical allowlist location (the
existing api/.imports_allowed reference already present later in the message),
ensuring the only allowlist instruction points to api/.imports_allowed.
🪄 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: Enterprise
Run ID: 7e63c228-99e9-48c2-beae-70adc281f6c3
📒 Files selected for processing (3)
api/.imports_allowedhack/tools/verify-api-deps/README.mdhack/tools/verify-api-deps/main.go
✅ Files skipped from review due to trivial changes (2)
- api/.imports_allowed
- hack/tools/verify-api-deps/README.md
b0934e9 to
21a5fb3
Compare
The more polluted the import list is, the harder it is for consumers to integrate HyperShift into their projects. For example, we used to have karpenter as a dependency, this meant that any HyperShift consumer also had karpenter as a dependency, and must now manage both the HyperShift dependency compatibility but also the karpenter dependency compatibility. It is best practice not to include implementation in the API module and to keep the module strictly to APIs.
… control This means that any changes have to be approved by an API reviewer
This means it doesn't matter from where you execture the binary. It will walk up the structure to find the root and then find the API module from there
21a5fb3 to
8ffd5a2
Compare
|
can we please include a ref to this make target here https://github.com/openshift/hypershift/blob/main/api/AGENTS.md#api-versioning |
It's already included in the verify, do you still want me to put it in separately as an early/quick check? |
yes please, just like we have api-lint, api-lint-fix... in that list so it has a list of targeted items without needing to run verify which is more expensive all the time but only once. |
Give the agents some guidance that utils and methods should not be included on API packages
|
/approve |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, JoelSpeed 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
|
/verified by @JoelSpeed |
openshift-ci-robot
added
the
verified
|
@JoelSpeed: This PR has been marked as verified 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. |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
Now I have the complete picture. Let me generate the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 11 test failures are Azure node provisioning timeouts in the Root CauseIntermittent Azure node provisioning failures — not related to PR #8364. The root cause is Azure infrastructure intermittently failing to provision new worker nodes for specific NodePools within the 45-minute timeout. The evidence shows:
Recommendations
Evidence
|
Test Resultse2e-aws
e2e-aks
|
|
@JoelSpeed: 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. |
bcd1884
into
openshift:main
4 participants
What this PR does / why we need it:
Follow up to #8355
The more polluted the import list is, the harder it is for consumers to integrate HyperShift into their projects. For example, we used to have karpenter as a dependency, this meant that any HyperShift consumer also had karpenter as a dependency, and must now manage both the HyperShift dependency compatibility but also the karpenter dependency compatibility.
It is best practice not to include implementation in the API module and to keep the module strictly to APIs.
This PR add
make verify-api-depsto prevent folks from adding new dependencies until/unless they convince an API reviewer that there is a need.Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Chores
Documentation