Add dependencycheck tool to address long running no-vendor-cycles test #93448
Conversation
k8s-ci-robot
added
release-note-none
|
/sig testing |
k8s-ci-robot
added
sig/testing
|
/test pull-kubernetes-files-remake |
|
/test pull-kubernetes-e2e-kind |
|
/retest |
|
Thanks for jumping on this, much appreciated. I need to double check whether the current impl detects module level transitive dependencies on k8s.io/kubernetes or staging repos even if the particular packages we use don't depend back on these repos. I think the source scanning technique would miss those. If we currently catch/prevent those, we shouldn't regress that. I'll queue this up to look at early next week. |
|
@liggitt thanks! I think that |
|
/uncc glad to see @liggitt on this! |
looks like we only checked packages found in vendor previously if possible, I'd like to lean on A go program to process that deps.json file seems better than bash I would envision something like:
As a follow-up, for the module-level cycle checks, we can separately use |
|
@liggitt do we need to explicitly run 3 & 4 separately? This check will execute quickly, so it is not a huge deal, but we are iterating twice through when it seems like one iteration checking for both would be faster. |
|
it's a little easier to treat staging repos and k8s.io/kubernetes separately, but there's not a strong reason to. I expect the tool will take less than a second to run so I doubt calling it twice would matter |
|
@hasheddan @liggitt What's the next step here? I'm looking at verify-master failures and I just want to understand if there are more changes needed on this PR? |
|
@RobertKielty I need to refactor as has been suggested. I should have this up today. However, this is not currently causing the |
|
Thanks @hasheddan |
|
@liggitt updated with module checks |
|
|
||
| var ( | ||
| exclude = flag.String("exclude", `a^`, "skip packages pattern") | ||
| restrict = flag.String("restrict", `a^`, "restricted dependencies pattern") |
There was a problem hiding this comment.
leave empty and require to be set
There was a problem hiding this comment.
clarify in the help this is a regex and maybe give an example?
|
|
||
| var ( | ||
| exclude = flag.String("exclude", `a^`, "skip packages pattern") | ||
| restrict = flag.String("restrict", `a^`, "restricted dependencies pattern") |
There was a problem hiding this comment.
clarify in the help this is a regex and maybe give an example?
|
@liggitt I believe all comments have been addressed and this is ready for another pass 👍 |
|
/retest |
1 similar comment
|
/retest |
down from 5m18s in a recent run 🎉 |
| args := flag.Args() | ||
|
|
||
| if len(args) != 1 { | ||
| log.Fatalf("usage: dependencycheck <json-dep-file>") |
There was a problem hiding this comment.
nit, give an example of how to generate this file (e.g. go list -mod=vendor -test -deps -json ./vendor/...)
| log.Fatalf("Error compiling excluded package regex: %v", err) | ||
| } | ||
| } | ||
| fmt.Println(excludePattern) |
hack/verify-no-vendor-cycles.sh
Outdated
| exit 1 | ||
| fi | ||
| # Check for any module that is not main or staging and depends on main or staging | ||
| go mod graph | grep -vE "^k8s.io\/(kubernetes|${staging_repos_pattern})" | grep -E "\sk8s.io\/(kubernetes|${staging_repos_pattern})" && exit 1 |
There was a problem hiding this comment.
this will not output a helpful error if it fails
suggest something like
bad_deps=$(go mod graph | grep -vE "^k8s.io\/(kubernetes|${staging_repos_pattern})" | grep -E "\sk8s.io\/(kubernetes|${staging_repos_pattern})" || true)
if [[ -n "${bad_deps}" ]]; then
echo "Found disallowed dependencies that transitively depend on k8s.io/kubernetes or staging modules:"
echo "${bad_deps}"
exit 1
fi
hack/verify-no-vendor-cycles.sh
Outdated
| go run cmd/dependencycheck/dependencycheck.go -restrict "^k8s\.io/kubernetes/" "${KUBE_TEMP}/deps.json" | ||
|
|
||
| # Check for any vendored package that depends on staging | ||
| go run cmd/dependencycheck/dependencycheck.go -restrict "^k8s\.io/(${staging_repos_pattern})(/|$)" "${KUBE_TEMP}/deps.json" |
There was a problem hiding this comment.
I think both of these need to exclude staging repos from the checks:
-exclude "^k8s\.io/(${staging_repos_pattern})(/|$)"
| type goPackage struct { | ||
| Name string | ||
| ImportPath string | ||
| Deps []string |
There was a problem hiding this comment.
Deps includes all transitive dependencies of this package, which is likely to be enormous
Imports + TestImports + XTestImports is likely to be more useful in identifying the actual problematic import that needs to be dealt with
suggest:
--- a/cmd/dependencycheck/dependencycheck.go
+++ b/cmd/dependencycheck/dependencycheck.go
@@ -34,9 +34,11 @@ var (
)
type goPackage struct {
- Name string
- ImportPath string
- Deps []string
+ Name string
+ ImportPath string
+ Imports []string
+ TestImports []string
+ XTestImports []string
}
func main() {
@@ -86,7 +88,12 @@ func main() {
continue
}
importViolations := []string{}
- for _, d := range p.Deps {
+
+ allImports := []string{}
+ allImports = append(allImports, p.Imports...)
+ allImports = append(allImports, p.TestImports...)
+ allImports = append(allImports, p.XTestImports...)
+ for _, d := range allImports {
if depsPattern.MatchString(d) {
importViolations = append(importViolations, d)
}
|
Very cool, Dan! |
dependencycheck verifies that no violating depdendencies exist in pkgs passed via a JSON file generated from go list. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
verify-no-vendor-cycles currently routinely takes 3-4 minutes to run due to the way that it checks for cyclical imports in vendored packages. Moving to using the new dependencycheck tool makes this test much faster. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
|
/lgtm |
k8s-ci-robot
added
do-not-merge/hold
|
/test pull-kubernetes-e2e-kind |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hasheddan, liggitt 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 |
k8s-ci-robot
added
the
approved
|
/retest |
k8s-ci-robot
removed
the
do-not-merge/hold
|
thanks @hasheddan @liggitt |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Adds a new tool,
dependencycheck, to detect cyclical dependencies on main and staging repos in vendored packages. This is currently being handled byhack/verify-no-vendor-cycles.sh, which takes exceedingly long to run (3-4 minutes) for the task it is accomplishing.dependencychecktypically runs in a few seconds. In addition, when taking a look at the logs forverify-masterand trying it out locally, I am not certain that this script is actually accomplishing its goal. For instance, if a package depended onk8s.io/apimachinery/pkg/runtime, it appears it is being printed as such, yet the script is checking for a match ofk8s.io/kubernetes/vendor/k8s.iothenk8s.io/apimachinery. I am guessing this may have to do with dependencies formerly being printed by their full path, but I am not sure. See more here: https://storage.googleapis.com/kubernetes-jenkins/logs/ci-kubernetes-verify-master/1287005337945116672/build-log.txtWhich issue(s) this PR fixes:
Ref: #87807 #92937
Special notes for your reviewer:
I wasn't sure who to add to the
OWNERSfile here so I just picked some of the usual suspects./cc @BenTheElder @dims @liggitt
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: