Adding in deploy changes tests for vcluster #2051
Conversation
✅ Deploy Preview for vcluster-docs canceled.Built without sensitive environment variables
|
sowmyav27
force-pushed
the
deploy-changes
branch
28 times, most recently
from
8fe8bc5
to
0cc2efa
Compare
sowmyav27
force-pushed
the
deploy-changes
branch
from
9d8ba9c
to
37a31f5
Compare
sowmyav27
force-pushed
the
deploy-changes
branch
21 times, most recently
from
d657a9e
to
ece351f
Compare
sowmyav27
force-pushed
the
deploy-changes
branch
from
ece351f
to
b4e8df3
Compare
sowmyav27
force-pushed
the
deploy-changes
branch
from
b4e8df3
to
5190f8d
Compare
| - main | ||
| - v* | ||
| paths: | ||
| - "test/deploy_changes/**" |
There was a problem hiding this comment.
This means we only want to run these tests if a file in test/deploy_changes/ changes. I believe we want this to run more often than that or else its not testing functional code changes for vcluster.
| deployCmd.Stdout = stdout | ||
| err := deployCmd.Run() | ||
| framework.ExpectNoError(err) | ||
| return err == nil && strings.Contains(stdout.String(), "Switched active kube context to") |
There was a problem hiding this comment.
Would an error be normal here, if so which error? We don't want to poll on an error if its not going to change.
| checkCmd := exec.Command("vcluster", "list") | ||
| output, err := checkCmd.CombinedOutput() | ||
| framework.ExpectNoError(err) | ||
| return err == nil && strings.Contains(string(output), f.VclusterName) && strings.Contains(string(output), "Running") |
There was a problem hiding this comment.
I think this check is too general. Here is the output of my vcluster list:
NAME | NAMESPACE | STATUS | VERSION | CONNECTED | AGE
------------+--------------------------+---------+----------------+-----------+------------
vcluster2 | loft-default-v-vcluster2 | Running | 0.20.0-beta.15 | | 266h4m40s
vcluster | vcluster | Running | uLmOpUH | | 238h42m5s
Let's say instead vcluster2 was not in Running and f.VclusterName was vcluster2. The output still contains vcluster2 and Running. I think instead you may want to split by new line or, even better use a regex. I still would be worried about using contains with the former because you have to consider on a single line that f.VclusterName could be a substring of another vcluster name, like with vcluster and vcluster2.
| to: default/nginx | ||
| experimental: | ||
| syncSettings: | ||
| setOwner: true |
| ) | ||
|
|
||
| var _ = ginkgo.Describe("Deploy sync changes to vCluster", func() { | ||
| ginkgo.It("should deploy a vCluster using kubectl and verify sync changes ", func() { |
There was a problem hiding this comment.
This says deploy a vCluster but it seems there is one already deployed. Redeploy or update would be more accurate.
There was a problem hiding this comment.
This is a pretty big change as that moves all tests to a different folder and I'm not sure this is what we want as e2e tests are pretty hidden now. I would like to eventually have a consistent naming across vcluster, vcluster-pro and loft-enterprise. Whats the reasoning behind moving them to functional_tests? I find the naming a little odd as we have now test -> functional_tests and test -> deploy_changes, which seems a little inconsistent to me, as one has tests in it and the other not. When looking at Kubernetes, they also do test -> e2e, so I would like to stay with that pattern, but we could introduce test -> deploy or something similar for your new tests
What issue type does this pull request address? (keep at least one, remove the others)
Tests -- Adding in deploy changes tests for vcluster