fix(#14): upgrade all CouchDb nodes #15
Conversation
|
Hi @nydr Would you mind having a look at this fix? |
| const containerPrefix = upgradeMessagePayload.containers[0].container_name; | ||
|
|
||
| await testContainerTag(upgradeService, `${containerPrefix}-1`, imageTag); | ||
| await testContainerTag(upgradeService, `${containerPrefix}-2`, imageTag); | ||
| }); | ||
|
|
||
| it('Doesnt error if JSON format and container field has additional fields', async () => { |
There was a problem hiding this comment.
Minor: This description could use an update as it's handling the additional fields rather than just not erroring. Also: Am I understanding it correctly that there's no remaining tests checking for failed upgrades? Not overly concerned by this as they're currently not handled as far as I know.
There was a problem hiding this comment.
This test still handles "failed" upgrades, just the output is changed, so we don't report on which container names weren't matched, which is fine, API doesn't expect any type of response when it calls the upgrade endpoint.
It's not exactly a failed upgrade, because it's not a demand to absolutely match all containers in the payload.
| }); | ||
| }); | ||
|
|
||
| it('Should upgrade deployment if container is named with a suffix', async () => { | ||
| const upgradeMessageArray: IUpgradeMessage[] = [{ container_name: 'busybox', image_tag: 'busybox:1.35' }]; | ||
| await runCommand(`sleep 5`, 'Avoid 409 conflict race condition with previous test'); |
There was a problem hiding this comment.
Minor: Is there a reason to use runCommand with sleep rather than setTimeout as you do above? I see it's being used already in this file and since it's only being used with already expensive tests so the minor overhead doesn't matter but just curious
There was a problem hiding this comment.
I was just being lazy and reusing existing code.
There was a problem hiding this comment.
I was also being lazy about resolving the race condition itself. I think reusing the same deployments in consecutive tests is pretty bad, because a failure can cascade to other tests, so the correct fix would be to use different containers for every test.
Co-authored-by: Daniel <nydr@users.noreply.github.com>
#14