-
Notifications
You must be signed in to change notification settings - Fork 87
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
KEP-0008: Multi-cluster - Proposals for assertion, errors Changes #343
base: main
Are you sure you want to change the base?
KEP-0008: Multi-cluster - Proposals for assertion, errors Changes #343
Conversation
…rt multi-cluster tests. Signed-off-by: Miles-Garnsey <[email protected]>
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.
good proposal... some of my comments may be answered in other concurrent PRs. This was the first.
@@ -33,6 +33,14 @@ Currently, KUTTL only supports a single test cluster. This KEP describes how we | |||
* Support applying resources across more than one test cluster. | |||
* Support asserting on resources across more than one test cluster. | |||
|
|||
# Proposal - Testassert and test errors | |||
|
|||
There is already a `TestAssert`` API which allows the test assertions to be configured. We propose to add to this a field `runOnCluster` of type `[]string` which will specify a set of named contexts on which to run the assertion. The strings will link back to the keys of the map defined in the proposed `MultiClusterConfig` struct, or (if a global config has been specified with the `NumClusters` option), to the KinD clusters deployed by the `TestSuite` setup steps. |
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.
There is already a `TestAssert`` API which allows the test assertions to be configured. We propose to add to this a field `runOnCluster` of type `[]string` which will specify a set of named contexts on which to run the assertion. The strings will link back to the keys of the map defined in the proposed `MultiClusterConfig` struct, or (if a global config has been specified with the `NumClusters` option), to the KinD clusters deployed by the `TestSuite` setup steps. | |
There is already a `TestAssert` API which allows the test assertions to be configured. We propose to add to this a field `runOnCluster` of type `[]string` which will specify a set of named contexts on which to run the assertion. The strings will link back to the keys of the map defined in the proposed `MultiClusterConfig` struct, or (if a global config has been specified with the `NumClusters` option), to the KinD clusters deployed by the `TestSuite` setup steps. |
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.
it is now more clear on proposed direction...
wondering if it would be reasonable to just define cluster
as opposed to runOnCluster
Also... It seems reasonable to have the test configuration phase define the TestAssert (and the "cluster lookup") so there isn't a look up at time of run... which may change the name runOnCluster
differently.
One issue I see is at startup we hit the defined cluster and create a kubeconfig
locally. There is an issue reported to make sure that is cleaned up. What we will need to define is: we will use a locally stored file... we they have different names and how (perhaps in the multi-cluster-config). What may be usefully to understand is the kubeconfig file is used I believe for different purposes but largely for cmd executions kubectl xyz
. We will need to work thru how the commands will be configured while support more than 1 cluster.
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.
wondering if it would be reasonable to just define cluster as opposed to runOnCluster
Yes, I think runOnCluster
is clumsy. Probably calling the field Clusters
is good, (*especially if we're to make this an array, note that's what I'm thinking!)
We will need to work thru how the commands will be configured while support more than 1 cluster.
Some discussion of that here:
One outstanding question, is whether (in addition to making environment variables available in scripts and commands) we should have some sort of automatic kubeconfig/kube context injection mechanism, so that a kubectl command which does not explicitly use the KUBECONFIG or KUBECONTEXT environment variables automatically has the appropriate argument inserted. If we did include this, we should also include a field to disable that functionality.
So I'm thinking that we should make KUBECONFIG and KUBECONTEXT env variables available, but also potentially inject those variables automatically for kubectl
commands. I'd value your thoughts there, since I'm pretty sure existing functionality already does something like that.
What this PR does / why we need it:
KEP-0008 is currently light on detail. To get it ready for implementation this PR fleshes out proposed changes to the assertions and errors APIs.
The changes proposed are designed to allow a user to specify what clusters assertions and errors will run on.