bootstrap script should be executed on the host agent #41
Conversation
Signed-off-by: chen hui <[email protected]>
Signed-off-by: chen hui <[email protected]>
Signed-off-by: Hui Chen <[email protected]>
Signed-off-by: chen hui <[email protected]>
Signed-off-by: chen hui <[email protected]>
Signed-off-by: chen hui <[email protected]>
| @@ -53,14 +58,14 @@ var _ = Describe("Agent", func() { | |||
| command := exec.Command(pathToHostAgentBinary, "--kubeconfig", kubeconfigFile.Name(), "--namespace", ns.Name) | |||
| session, err = gexec.Start(command, GinkgoWriter, GinkgoWriter) | |||
| Expect(err).ToNot(HaveOccurred()) | |||
| Eventually(session).Should(gexec.Exit(255)) | |||
| Eventually(session).Should(gexec.Exit(0)) | |||
There was a problem hiding this comment.
The It description says it should error out, then why Exit(0)?
There was a problem hiding this comment.
That's because I changed all klog.Fatal to "klog.Errorf and return"(comment from jamie).
There was a problem hiding this comment.
Then should we assert on error message? To someone reading the test, this is confusing.
There was a problem hiding this comment.
There is no way to assert error message, since main function return no argument.
| }) | ||
|
|
||
| It("should return an error when invalid kubeconfig is passed in", func() { | ||
| command := exec.Command(pathToHostAgentBinary, "--kubeconfig", fakedKubeConfig) | ||
| session, err = gexec.Start(command, GinkgoWriter, GinkgoWriter) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Eventually(session).Should(gexec.Exit(255)) | ||
| Eventually(session).Should(gexec.Exit(0)) |
There was a problem hiding this comment.
That's because I changed all klog.Fatal to "klog.Errorf and return"(comment from jamie).
| @@ -110,28 +115,65 @@ var _ = Describe("Agent", func() { | |||
| }) | |||
|
|
|||
| It("should bootstrap the node when MachineRef is set", func() { | |||
|
|
|||
There was a problem hiding this comment.
For this particular test case, only one file is sufficient. Don't you think?
Asking because the test is for asserting if the node is bootstrapped when MachineRef is set, it should not matter if there are different flavors / combinations of cloudinit configs or not
There was a problem hiding this comment.
It can't test different scenarios on one file:
file1: Don't specify permissions+append, to see if the function works.
file2: Specify permissions+append, to see if the function works.
file3: Specify encoding in base64 way, to see if it can decoded in base64 way.
file4: Specify encoding in gzip+base64 way, to see if it can decoded in gzip+base64 way.
This is integration test of this story, to make sure the code is ok.
There was a problem hiding this comment.
I remember commenting earlier to combine certain test cases. I'm not sure if this is one of those. Apologies for that.
So I see two parts here -
- Whenever a MachineRef is set on the ByoHost, the node should be bootstrapped (here we are hoping cloudinit will pass through - so maybe don't care for different scenarios for that)
- Different scenarios for cloudinit itself - this feels more of multiple unit tests instead of testing all in one integration test
Thoughts? @huchen2021 @jamiemonserrate
There was a problem hiding this comment.
@anusha94 It was two It blocks, you suggested to combine it. So I combine it, and put it in different files.
I think it's no big deal about whether it is one block, or two blocks, as long as the code are all covered by test case.
Co-authored-by: Anusha Hegde <[email protected]>
Co-authored-by: Anusha Hegde <[email protected]>
Co-authored-by: Anusha Hegde <[email protected]>
Co-authored-by: Anusha Hegde <[email protected]>
Co-authored-by: Anusha Hegde <[email protected]>
Co-authored-by: Anusha Hegde <[email protected]>
Co-authored-by: Anusha Hegde <[email protected]>
bootstrap script should be executed on the host agent
jamiemonserrate
changed the title
Signed-off-by: chen hui [email protected]