-
Notifications
You must be signed in to change notification settings - Fork 1.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
🌱 fix the breaking change to still allowing TestContext to be used for tests outside of the cluster #1883
🌱 fix the breaking change to still allowing TestContext to be used for tests outside of the cluster #1883
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 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 |
Can you describe why you wouldn’t want cluster version lookup in the constructor? This will only panic if cluster version isn’t available (no running cluster), which is a nice fail-early check. There’s a fix you can make such that an error occurs instead of a panic, instead of removing version lookup from the constructor. /hold |
6fc5db5
to
486c7f0
Compare
486c7f0
to
9c96ab2
Compare
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.
Cluster version is required by other e2e setups steps, which is why the lookup exists in the constructor. TestContext
is meant for live cluster testing, so having a running cluster requirement is reasonable.
Hi @estroz,
It is not true. It is ONLY required on the current tests after the changes made in #1840 to use a different version of cert-manager. Cannot we write a test that does not use cert-manager at all? Should we not probably able to remove the K8SVersion in the future if we wish when we no longer need to run the tests against k8s versions 1.16? So, how it can now be mandatory for ANY test done with TestContext?
The change made in #1840 defines that TestContext can ONLY be used when is possible to get the K8SVersion which I cannot agree that is reasonable. All tests implemented on this repository that uses the TestContext currently runs on the cluster, however, it does not mean that TestContext can ONLY be used to run a test on Cluster and that it is its ONLY purpose. And then, can we not have an e2e tests that not run on the cluster? I'd assume that it is in Conclusion; I cannot see any advantage and value by adding this limitation whcih in POV is indeed an issue introduced by #1840 or at least a bad practice to start to make mandatory values that not are really required for ANY and ALL possible tests scenarios in the constructor. However, I let it up to you. If you still think that we should not merge this one feel free to close it. |
Description
After the changes made in ea3a94a#diff-7b614da29899d7d7eb4aa3b60ed598604da518c4f2dd1e6af954c942fb1c3dc3 in many scenarios, we have been facing panic issues when it was calling the kubectl.Version(). See that TextContext has *kubectl which can be nil.
Note that, we ought to just look for the k8sVersion when/if it is required. It is not a mandatory attrs at all we might have tests that require knowing the k8sVersion and others that not.