Updation of import-restrictions file #115296

yashsingh74 · 2023-01-24T14:34:09Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The import-boss script file requires the .import-restrictions file. This will help the maintainers keep the dependencies clean. It is described as a YAML or JSON format. This file is not included in the CI runs. When it is added, the list of directories and subdirectories is also added.

As there is a list of directories and subdirectories that do not exist in the k/k repo. This PR helps to clean up the path to those directories and adds a list of directories that exist in the k/k path.

Ref: import-boss

Which issue(s) this PR fixes:

Fixes #115234

k8s-ci-robot · 2023-01-24T14:34:11Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2023-01-24T14:34:17Z

Welcome @yashsingh74!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-01-24T14:34:18Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2023-01-24T14:34:18Z

Hi @yashsingh74. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

yashsingh74 · 2023-01-24T14:43:36Z

@pohly Please review the PR.

pohly

Can you add comments explaining why you are making these changes and why these (and just these entries) are correct? Start commenting the file and then add further information in the commit message.

Assume that I don't have a clue how import-boss works (because I don't...).

Also note that there is a TODO in the file. Can that be resolved?

pohly · 2023-01-24T15:35:03Z

test/e2e/framework/.import-restrictions

@@ -1,116 +1,454 @@
 rules:
  - selectorRegexp: k8s[.]io/kubernetes/pkg/


Why is there a rule for k8s[.]io/kubernetes/pkg/ in test/e2e/framework/.import-restrictions? Does that limit which code may be used inside k8s.io/kubernetes/pkg? If yes, it doesn't seem like it belongs here.

import boss checks the packages in place using this format. Since just the packages used by the framework are being imported, not all of the k/k packages.
Also, we are using the defined path to import the pkg and test path instead of the 'k8s[.]io' parent path.

By looking just at this file and PR, it's still not clear to me what is being matched and what the effect is. Can you perhaps add comments to the file?

I probably also have to read up on how import boss works, but even then having those comments in the file will help the next person reading it.

I have updated which packages are used in the test and fixes the external Packages which are used in the test.
Also, the packages only includes the allowed prefix.

I checked https://pkg.go.dev/k8s.io/gengo/examples/import-boss, but I am still trying to wrap my head around it.

So here we make a statement about importing some package under k8s[.]io/kubernetes/pkg: only the packages listed explicitly are allowed?

This is a surprisingly long list - much too long IMHO. I suspect this is a result of using this .import-restrictions file for the core framework and for the domain-specific sub-packages.

I believe what we want is a fairly tight list here and then individual .import-restrictions files in the sub-package were we allow additional packages on a per-sub-package basis.

When you extended this list, how did you decide which packages to list? Are all of them really needed somewhere or did you list everything that currently exists?

…s that exist in the k/k path. Signed-off-by: Yash Singh <syash@vmware.com>

pohly · 2023-02-07T12:40:52Z

test/e2e/framework/.import-restrictions

@@ -1,116 +1,454 @@
 rules:
  - selectorRegexp: k8s[.]io/kubernetes/pkg/


I checked https://pkg.go.dev/k8s.io/gengo/examples/import-boss, but I am still trying to wrap my head around it.

So here we make a statement about importing some package under k8s[.]io/kubernetes/pkg: only the packages listed explicitly are allowed?

This is a surprisingly long list - much too long IMHO. I suspect this is a result of using this .import-restrictions file for the core framework and for the domain-specific sub-packages.

I believe what we want is a fairly tight list here and then individual .import-restrictions files in the sub-package were we allow additional packages on a per-sub-package basis.

When you extended this list, how did you decide which packages to list? Are all of them really needed somewhere or did you list everything that currently exists?

pohly · 2023-02-07T12:46:33Z

test/e2e/framework/.import-restrictions

  - selectorRegexp: k8s[.]io/kubernetes/test/
    allowedPrefixes:
+      - k8s.io/kubernetes/test/conformance/testdata


I don't think we need to (and shouldn't) list every single package here.

What we want to say is "anything under test/e2e/framework may import anything under test". Or at least I can't think of something that shouldn't be allowed.

If that is the goal, then can't we just use this:

# Everything under k8s.io/kubernetes/test/ may be used. - selectorRegexp: k8s[.]io/kubernetes/test/ allowedPrefixes: - ""

Or even leave the rule out entirely?

According to the first comment, I have listed down all the packages which are being used in the test package. Yes, the list is too long so it is better to only keep prefix which are being used.

Second, in the */test package we can use the suggested changes as most of the import are used.

I will make the changes.

Also, I have only keep those packages which are only imported and used in the k8s.io/kubernetes/test/e2e/framework.

pohly · 2023-02-07T12:48:29Z

test/e2e/framework/.import-restrictions

  - selectorRegexp: k8s[.]io/kubernetes/third_party/
    allowedPrefixes:
-    - k8s.io/kubernetes/third_party/forked/golang/expansion
+    - k8s.io/kubernetes/third_party/forked/golang/net


According to my comment above, this would belong into test/e2e/network/.import-restrictions.

pohly

I tried out hack/verify-import-boss.sh with your branch and it is failing for me:

E0207 17:25:02.971904   76898 main.go:41] Error: Failed executing generator: some packages had errors:
errors in package "k8s.io/kubernetes/test/e2e/framework/providers/aws":
the following imports did not match any allowed prefix:
  k8s.io/kubernetes/pkg/api/legacyscheme
  k8s.io/kubernetes/pkg/apis/apps
  k8s.io/kubernetes/pkg/apis/autoscaling
...

Same for several sub-packages.

I think we need to override the restrictions for k8s.io/kubernetes/pkg for sub-packages.

If I add the following file to several directories, the check passes:

rules:
  # Allow full usage of the entire k8s.io/kubernetes/pkg.
  # This is probably too broad and could be restricted further,
  # but that isn't important. What matters are the restrictions
  # of the core framework: those need to be overridden here
  # due to how the E2E packages are structured.
  - selectorRegexp: k8s[.]io/kubernetes/pkg/
    allowedPrefixes:
    - ""

	test/e2e/framework/daemonset/.import-restrictions
	test/e2e/framework/kubelet/.import-restrictions
	test/e2e/framework/perf/.import-restrictions
	test/e2e/framework/providers/aws/.import-restrictions
	test/e2e/framework/providers/gce/.import-restrictions
	test/e2e/framework/providers/kubemark/.import-restrictions
	test/e2e/framework/pv/.import-restrictions

test/e2e/framework/.import-restrictions

k8s-ci-robot · 2023-02-11T14:24:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yashsingh74
Once this PR has been reviewed and has the lgtm label, please assign andrewsykim for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

test/e2e/framework/OWNERS
test/e2e/network/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yashsingh74 · 2023-02-11T14:26:51Z

@pohly I have updated the file and ran the hack/verify-import-boss.sh script. Now it doesn't show the same error.
The reason I included the allowedPrefix imports is because it is used in the test/e2e/framework. Also, removed the k8s[.]io/kubernetes/test/ selector.
Please confirm and let me know if it requires any more changes.

pohly · 2023-02-11T16:59:12Z

/ok-to-test

pohly · 2023-02-12T13:58:05Z

This is still not what I was aimining at:

test/e2e/framework/*.go should have very minimal dependencies. We need to 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.

Now that I understand how import-boss works, it's easier for me to spell it out with code than with words 😢

Sorry, but I think I'll solve #115234 myself. Can you perhaps review #115710?

yashsingh74 · 2023-02-12T14:00:34Z

Thank you @pohly I will look up for your open PR.

k8s-ci-robot · 2023-02-22T00:24:15Z

PR needs rebase.

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.

pohly · 2023-02-23T10:32:48Z

/close

#115710 was merged.

k8s-ci-robot · 2023-02-23T10:32:54Z

@pohly: Closed this PR.

In response to this:

/close

#115710 was merged.

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.

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 24, 2023

k8s-ci-robot requested review from fabriziopandini and SataQiu January 24, 2023 14:35

pohly suggested changes Jan 24, 2023

View reviewed changes

pohly reviewed Jan 24, 2023

View reviewed changes

yashsingh74 force-pushed the yash-import-fixes branch from 406c654 to 707fc60 Compare January 26, 2023 14:16

Cleans up the path to those directories and adds a list of directorie…

cc98e2a

…s that exist in the k/k path. Signed-off-by: Yash Singh <syash@vmware.com>

yashsingh74 force-pushed the yash-import-fixes branch from 707fc60 to cc98e2a Compare February 6, 2023 16:19

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 6, 2023

pohly suggested changes Feb 7, 2023

View reviewed changes

Updated the file with the suggested changes

55ce533

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Feb 7, 2023

Updated the file with the suggested changes

12e5449

pohly suggested changes Feb 7, 2023

View reviewed changes

test/e2e/framework/.import-restrictions Outdated Show resolved Hide resolved

test/e2e/framework/.import-restrictions Outdated Show resolved Hide resolved

Added the suggested changes

b64ac56

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 11, 2023

Added the suggested changes

90feb54

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 12, 2023

pohly mentioned this pull request Feb 12, 2023

e2e framework: revise import restrictions #115710

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2023

k8s-ci-robot closed this Feb 23, 2023

yashsingh74 deleted the yash-import-fixes branch February 23, 2023 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updation of import-restrictions file #115296

Updation of import-restrictions file #115296

yashsingh74 commented Jan 24, 2023 •

edited

k8s-ci-robot commented Jan 24, 2023

k8s-ci-robot commented Jan 24, 2023

k8s-ci-robot commented Jan 24, 2023

k8s-ci-robot commented Jan 24, 2023

yashsingh74 commented Jan 24, 2023

pohly left a comment

pohly Jan 24, 2023

yashsingh74 Jan 26, 2023

pohly Jan 26, 2023

yashsingh74 Feb 6, 2023

pohly Feb 7, 2023

pohly Feb 7, 2023

pohly Feb 7, 2023

yashsingh74 Feb 7, 2023

yashsingh74 Feb 7, 2023

pohly Feb 7, 2023

yashsingh74 Feb 7, 2023

pohly left a comment

k8s-ci-robot commented Feb 11, 2023

yashsingh74 commented Feb 11, 2023

pohly commented Feb 11, 2023

pohly commented Feb 12, 2023

yashsingh74 commented Feb 12, 2023

k8s-ci-robot commented Feb 22, 2023

pohly commented Feb 23, 2023

k8s-ci-robot commented Feb 23, 2023

		@@ -1,116 +1,454 @@
		rules:
		- selectorRegexp: k8s[.]io/kubernetes/pkg/

Updation of import-restrictions file #115296

Updation of import-restrictions file #115296

Conversation

yashsingh74 commented Jan 24, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

k8s-ci-robot commented Jan 24, 2023

k8s-ci-robot commented Jan 24, 2023

k8s-ci-robot commented Jan 24, 2023

k8s-ci-robot commented Jan 24, 2023

yashsingh74 commented Jan 24, 2023

pohly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 11, 2023

yashsingh74 commented Feb 11, 2023

pohly commented Feb 11, 2023

pohly commented Feb 12, 2023

yashsingh74 commented Feb 12, 2023

k8s-ci-robot commented Feb 22, 2023

pohly commented Feb 23, 2023

k8s-ci-robot commented Feb 23, 2023

yashsingh74 commented Jan 24, 2023 •

edited