Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Miles-Garnsey
Copy link

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.

…rt multi-cluster tests.

Signed-off-by: Miles-Garnsey <[email protected]>
Copy link
Member

@kensipe kensipe left a 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member

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.

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants