-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
e2e framework: revise import restrictions #115710
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
- test/e2e/framework/*.go should have very minimal dependencies. We can enforce that via import-boss. - What each test/e2e/framework/* sub-package uses is less relevant, although ideally it also should be as minimal as possible in each case. Enforcing this via import-boss ensures that new dependencies get flagged as problem and thus will get additional scrutiny. It might be okay to add them, but it needs to be considered.
883e904
to
3e76031
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/assign @pohly |
/assign @oomichi You might still have some context on why the import restrictions were added initially? |
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.
@pohly
Thanks for doing that.
I see the point of this pull request.
/lgtm
# which means that it shouldn't depend on internal | ||
# code. But we are not there yet, so some exceptions | ||
# have to be allowed. Over time the list of allowed | ||
# packages should get shorter, not longer. |
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.
I agree with the above comment.
We tried to solve the dependency issue so hardly, but we saw that is hard to do that actually.
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.
Almost done now 😁
The last remaining issue is embedding code that is only used by test/e2e_node inside test/e2e/framework (mostly for the TestContext). That pulls in the k/k/pkg dependency.
LGTM label has been added. Git tree hash: 43fc520b7b0fe6676cb5e2cf9746b9b60b5a1b8f
|
/test pull-kubernetes-e2e-gci-gce-ipvs |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
test/e2e/framework/*.go should have very minimal dependencies. We can enforce that via import-boss.
What each test/e2e/framework/* sub-package uses is less relevant, although ideally it also should be as minimal as possible in each case.
Enforcing this via import-boss ensures that new dependencies get flagged as problem and thus will get additional scrutiny. It might be okay to add them, but it needs to be considered.
Which issue(s) this PR fixes:
Fixes #115234
Special notes for your reviewer:
See #115296 for some prior discussion.
Does this PR introduce a user-facing change?