chore(e2e): add e2e tests for sidecars#1011
Conversation
aa03d75 to
010c533
Compare
kohidave
left a comment
There was a problem hiding this comment.
This looks so delightful! Thank you :)
| @@ -0,0 +1,42 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Might be nice to have an AwsCLI struct, like we have for the CopilotCLI
There was a problem hiding this comment.
Maybe it's not a good idea since the reason why we have a struct for copilot is because of mocking.
| It("create new ECR repo for sidecar", func() { | ||
| var b bytes.Buffer | ||
| err := command.Run("bash", []string{"-c", fmt.Sprintf("aws ecr create-repository --repository-name %s | jq -r .repository.repositoryUri", sidecarRepoName)}, command.Stdout(&b)) | ||
| Expect(err).NotTo(HaveOccurred(), "create ECR repo for sidecar") | ||
| uri = strings.TrimSpace(b.String()) | ||
| sidecarImageURI = fmt.Sprintf("%s:%s", uri, tag) | ||
| }) | ||
| It("push sidecar image", func() { | ||
| var b bytes.Buffer | ||
| err := command.Run("aws", []string{"ecr", "get-login-password"}, command.Stdout(&b)) | ||
| password := b.String() | ||
| Expect(err).NotTo(HaveOccurred(), "get ecr login password") | ||
| err = command.Run("docker", []string{"login", "-u", "AWS", "--password-stdin", uri}, command.Stdin(strings.NewReader(password))) | ||
| Expect(err).NotTo(HaveOccurred(), "docker login") | ||
| err = command.Run("docker", []string{"build", "-t", sidecarImageURI, "./nginx"}) | ||
| Expect(err).NotTo(HaveOccurred(), "build sidecar image") | ||
| err = command.Run("docker", []string{"push", sidecarImageURI}) | ||
| Expect(err).NotTo(HaveOccurred(), "push to ECR repo") |
There was a problem hiding this comment.
Do you think this should be a BeforeAll of the 'write manifest file' section?
There was a problem hiding this comment.
Yeah that's a good idea!
There was a problem hiding this comment.
i tried to use BeforeAll but it seems like inside of beforeAll it doesn't allow me to have multiple It(). so i'll leave it as it is now.
| const lorem = require('lorem-ipsum') | ||
|
|
||
| // Log requests | ||
| app.use(function *(next){ |
There was a problem hiding this comment.
Wow it's the first time that I see the generator syntax in JS! :O
|
|
||
| for _, logLine := range svcLogs { | ||
| Expect(logLine.Message).NotTo(Equal("")) | ||
| Expect(logLine.LogStreamName).NotTo(Equal("")) |
There was a problem hiding this comment.
maybe we can validate that the logstreamname is the custom one that firelens set for us?
There was a problem hiding this comment.
But the thing is it contains not only the logs from firelens but also nginx logs and the logs for firelens itself...
There was a problem hiding this comment.
maybe i can try to check if the log stream name falls into these three options. But still we don't know the full name for the log streams unless we query it.
9821825 to
02b0666
Compare
Fixes #1008.
(I'm gonna rest in peace. Put the e2e test log on my grave and don't include any PR reviews other than LGTM)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.