-
Notifications
You must be signed in to change notification settings - Fork 612
Separate conformance tests dependencies #4188
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rikatz 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 |
kflynn
left a comment
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.
This looks really good, thanks! One question from my end.
robscott
left a comment
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.
Thanks @rikatz, this is great!
| k8s.io/client-go v0.34.1 | ||
| k8s.io/kube-openapi v0.0.0-20250814151709-d7b6acb124c3 | ||
| k8s.io/utils v0.0.0-20250820121507-0af2bda4dd1d | ||
| sigs.k8s.io/controller-runtime v0.22.3 |
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.
What's left that still requires a controller-runtime dependency?
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.
the pkg/test uses envtest, which is part of controller-runtime.
Probably as part of the VAP tests effort we can split it as well, and get rid once for all of controller-runtime as an API dependency
dprotaso
left a comment
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 believe introducing a new go module into the repository will now require additional changes to the release process
See: https://go.dev/doc/modules/managing-source?utm_source=chatgpt.com#multiple-module-source
|
|
||
| go 1.24.0 | ||
|
|
||
| replace sigs.k8s.io/gateway-api => ../ |
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.
Technically you don't need this given you have the go.work folder
and allow generating a binary to run conformance on a different machine
Also having the replace will prevent go install and go run from working.
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.
what's the case of having "go install" or "go run" here as we don't have a "main"?
I agree that replacement may be problematic in case we try to do some "go install", but "go run" works fine:
go run ./tools/gepstoc/main.go
the "tools" directory has its own go.mod already. Did I missed something?
| k8s.io/client-go v0.34.1 | ||
| k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 | ||
| sigs.k8s.io/controller-runtime v0.22.3 | ||
| sigs.k8s.io/gateway-api v0.0.0-00010101000000-000000000000 |
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.
Curious what this pseudo version means exactly?
In order for the build to work I think we'll need to have automation to update this module version
eg.
- A commit makes a change to the
sigs.k8s.io/gateway-apimodule - Automation then needs to bump the version in this module file
Unsure if we want to do this as part of every release (more error prone) or every commit (then you could build the conformance binary at HEAD). One caveat is we'd want to make sure this doesn't trigger an infinite loop.
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.
The pseudo version is related to the replacement we are doing, mostly saying "we don't care, use the latest from the replacement"
Similar to Kubernetes main repo: https://github.com/kubernetes/kubernetes/blob/master/go.mod#L88
As we are not building binaries, but APIs, it makes sense that we keep the "go.mod" of the API as clean as possible, with the "cost" IMO of the inner modules that we don't provide to consumers with these kind of tweaks.
f56f22b to
5567d48
Compare
|
@dprotaso btw, as you are here, do you mind trying to build knative with the changes above? I may try doing it as well with knative and other implementations, mostly willing to see if something will break on someone's build env |
5567d48 to
48626b4
Compare
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Separate conformance tests dependencies from main tests, and allow generating a binary to run conformance on a different machine
Which issue(s) this PR fixes:
Fixes #3318
Does this PR introduce a user-facing change?: