rollouts: end to end tests #3879
Conversation
| cnrm.cloud.google.com/system: "true" | ||
| cnrm.cloud.google.com/tf2crd: "true" | ||
| core.cnrm.cloud.google.com/configconnector: configconnector.core.cnrm.cloud.google.com | ||
| name: containerclusters.container.cnrm.cloud.google.com |
There was a problem hiding this comment.
Does this file come from another Github source? We could consider using the remote URL in the kustomization in that case, instead of copying it over
There was a problem hiding this comment.
I want to find a better solution for this CRD. This is an external CRD (comes from KCC) and is already part of our release manifests, I will try to reuse that instead of adding a new one here.
There was a problem hiding this comment.
I am reusing the containercluster.yaml from the manifests directory for now. So we have one source of truth for now. Eventually we want to make rollouts to be able to gracefully handle unavailability of this CRD.
droot
force-pushed
the
rollouts-integration-tests
branch
from
42514ab
to
3c7e341
Compare
| err := testEnv.Stop() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| _ = testEnv.Stop() | ||
| // Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Was this an intentional change? If so, curious why we are doing this?
There was a problem hiding this comment.
Yes, this is an intentional change. Added a note with a TODO explaining why we are ignore this assertions.
| Prefix: "e2e-sfoo-", | ||
| Count: 1, | ||
| Labels: map[string]string{ | ||
| "city": "sfoo", |
There was a problem hiding this comment.
Why sjcc and sfoo instead of sjc and sfo? Is there some conflict with the clusters setup in the previous test?
Since this is otherwise the same function passed to BeforeEach as the previous test, I was wondering if it would make sense to define this func elsewhere and reuse it across tests.
There was a problem hiding this comment.
We want these two tests to be independent of each other so that we can run them in parallel (though running in parallel requires running with ginkgo CLI, but that's a different story). Since we share the APIServer/Etcd between the two tests, keeping a separate target clusters is required to keep these tests independent.
Also, each tests may require different target cluster environment (for ex. progressive may have more advance test setup here), so keeping them separate gives that flexibility. So duplication here is the feature not a bug :)
| targetClusterSetup, err = e2eclusters.GetClusterSetup(tt, k8sClient, targets...) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(targetClusterSetup.PrepareAndWait(context.TODO(), 5*time.Minute)).To(Succeed()) |
There was a problem hiding this comment.
Are we expecting to use 5*time.Minute in every test? If so, maybe we can put it in a const
There was a problem hiding this comment.
So these timeouts really depend on the target cluster setup and that may vary between these tests, so avoiding the reuse.
|
@natasha41575 responded to your comments, pl. take another look and let me know. |
This PR does the following:
I think this sets up our e2e test story nicely (except the git part), It should be easy to add new tests now.