Improve logic used to detect changes in Deployment resource #46
Conversation
Signed-off-by: Jonathan West <[email protected]>
jgwest
force-pushed
the
improve-deployment-update-logic-mar-2024
branch
from
6285ada
to
f82834e
Compare
jgwest
changed the title
Signed-off-by: Jonathan West <[email protected]>
| - name: Run tests | ||
| run: | | ||
| set -o pipefail |
There was a problem hiding this comment.
I think removing the set -o pipefail will mean that even if the tests fail, the tee command will succeed and so the overall command will have a zero exit status and therefore the failure won't be reflected in the status of the github action (i.e. the action will be seen as successful)
controllers/deployment.go
Outdated
| return appsv1.Deployment{}, fmt.Errorf("incorrect volume mounts") | ||
| } | ||
|
|
||
| // String slices need to be converted to nil, because reflect.DeepEqual(nil, []string{}) is false, despite being functionally the same, here. |
There was a problem hiding this comment.
The code is actually ensuring that any nil string slice is converted to an empty string slice, so the comment probably should be changed to say something like
// Nil string slices need to be converted to empty string slices, because reflect.DeepEqual(nil, []string{}) is false, despite being functionally the same, here.
controllers/deployment_test.go
Outdated
| By("ensuring the reconcileRolloutsDeployment will detect and revert the change") | ||
| { | ||
| By("calling reconcileRolloutsDeployment to create a default Deployment") | ||
| reconciledDepl := appsv1.Deployment{ |
There was a problem hiding this comment.
Not a big deal, but it might make things a bit easier to understand if reconciledDepl was called expectedDepl instead.
| for _, line := range fileLines[lineIndexStart:] { | ||
| // example: DONE 6 runs, 144 tests, 6 skipped, 47 failures in 2279.668s | ||
| if strings.HasPrefix(line, "DONE ") || time.Now().After(expireTime) { | ||
| fmt.Println("E2E tests file is complete:", line) |
There was a problem hiding this comment.
Maybe it would be better to have a separate message for when we're exiting due to a timeout?
Signed-off-by: Jonathan West <[email protected]>
jgwest
force-pushed
the
improve-deployment-update-logic-mar-2024
branch
from
0d317f8
to
4221242
Compare
|
👍 to all review comments, and all addressed. Thanks @jopit! |
What does this PR do / why we need it:
Deployment changes
Related E2E test changes:
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #47
How to test changes / Special notes to the reviewer: