-
Notifications
You must be signed in to change notification settings - Fork 4k
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
VPA: separate module for e2e tests #2531
Conversation
cc: @tariq1890, @bskiba |
b965245
to
d178dbb
Compare
I'm in favor of this change. Thank you for taking time to do this for us. To make it easier for me to review this, can you split the change into 3 commits:
One additional thing that needs changing is the Makefiles, we need to remove the godeps from building. However, I can do it in a followup PR. This one is huge as it is :) |
One more thing, I'd rather stick with go 1.12 for now if that's ok. |
- remove absolute paths with replace directives - split modules for e2e tests
d178dbb
to
56a1c7a
Compare
56a1c7a
to
50511dd
Compare
Thanks for a quick feedback. I split the PR out to multiple commits for easier review and added the k8s deps update script - 56a1c7a. With other dependencies, it should be fine using standard
I agree that a followup PR makes sense here if it isn't causing any problems in the interim |
This is so good, thank you! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bskiba 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 |
The `set` in `hack/update-kubernetes-deps-in-e2e.sh` contains redundant options: * `errexit` equals `-e` * `nounset` equals `-u` Also, I have misunderstood how to correctly set options in: kubernetes#2531 Resource: https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html
The `set` in `hack/update-kubernetes-deps-in-e2e.sh` contains redundant options: * `errexit` equals `-e` * `nounset` equals `-u` Also, I have misunderstood how to correctly set options in: kubernetes#2531 Resource: https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html
The `set` in `hack/update-kubernetes-deps-in-e2e.sh` contains redundant options: * `errexit` equals `-e` * `nounset` equals `-u` Also, I have misunderstood how to correctly set options in: kubernetes#2531 Resource: https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html
The `set` in `hack/update-kubernetes-deps-in-e2e.sh` contains redundant options: * `errexit` equals `-e` * `nounset` equals `-u` Also, I have misunderstood how to correctly set options in: kubernetes#2531 Resource: https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html
The `set` in `hack/update-kubernetes-deps-in-e2e.sh` contains redundant options: * `errexit` equals `-e` * `nounset` equals `-u` Also, I have misunderstood how to correctly set options in: kubernetes#2531 Resource: https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html
Effort to simplify go modules tracking even more than #2530 by removing the requirement for replace directives with filesystem absolute paths and separate go module for the e2e tests.
It is generally not recommended to depend on
k8s.io/kubernetes
kubernetes/kubernetes#79384 (comment), and opens doors to issues for people who may want to depend on VPA - kubernetes/kube-state-metrics#949 (comment)It looks like the e2e tests are the only large obstacle in removing the dependency on
k8s.io/kuberentes
. So I would like to propose separate go module for the e2e tests to simplify depending on VPA and only make life hard for those who decide to depend on VPA e2e package. Input from maintainers is greatly appreciated.checking imports - grep output
Closes: #2507