-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Import Boss API restrictions should be transitive #9483
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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Makes sense!
/lgtm
/area ci Were you able to get the script to fail for transitive dependencies when using this? It doesn't seem to fail for me for the utils import in the |
I'd still have expected that to fail for the ones whcih are not fixed? /lgtm cancel |
So i'm trying to test this, but I can't actually get the import-boss to fail on anything at the moment, which is worrying /hold until I can work out how to get it to fail on importing a controller-runtime package |
I was about to ask this - I haven't actually reviewed any PRs with it until now. |
So Import Boss, like most other gengo related projects, does not like relative paths, which is how we are configuring it right now. When we pass When it traverses the Currently it's comparing proper import paths with relative paths and nothing is actually being verified. |
AFAIK we had it at least working at CAPV: There's an important caveat: kubernetes-sigs/cluster-api-provider-vsphere#2338 (comment) That was an important part when @sbueringer tested it afaik: |
@chrischdi Can you get a positive failure on CAPV? I just tried adding some debug logic to the skip and I don't think, even with the
The first string is the package name of a package we included in input dirs, but the logic here is causing it to skip the package |
I've been playing with various combinations and stepping through import-boss with a debugger, I can see no way that this can possibly work with relative paths, every combination of GOPATH set/unset and relative and package names, always ends up with the package name being compared against the input dir string. My conclusion so far is that you have to have import-boss inside GOPATH for it to work, I hope someone can prove me wrong 😓 |
/close The transitive restrictions thing is for incoming imports, I'm not sure entirely what the use case is for that but it isn't going to help us here. I think we could potentially use the incoming import thing to mark up other packages and prevent them from being imported into the API packages. Eg, we could prevent the util package being imported by putting a restriction on the util package |
@JoelSpeed: Closed this PR. In 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/test-infra repository. |
Sorry might have missed it. Not sure if you were able to make it fail now. Once we used unset we were able to make it fail (in Prow and locally on macOS with M2 inside of the "regular" GOPATH location) kubernetes-sigs/cluster-api-provider-vsphere#2344 (comment)
|
What this PR does / why we need it:
We need to make sure that the packages transitive dependencies also don't import controller runtime for proper cleanliness and to prevent regressions.
Part of #9011
cc @sbueringer
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #