cli: perform upgrades in-place in Terraform workspace #2317
Conversation
✅ Deploy Preview for constellation-docs canceled.
|
Coverage report
|
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signe 8000 d-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
5035dc7 to
0617c3a
Compare
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
| @@ -83,6 +83,15 @@ func TestUpgradeApply(t *testing.T) { | |||
| wantErr: true, | |||
| stdin: "no\n", | |||
| }, | |||
| "abort, rollback terraform err": { | |||
| kubeUpgrader: &stubKubernetesUpgrader{ | |||
| currentConfig: config.DefaultForAzureSEVSNP(), | |||
There was a problem hiding this comment.
can we check that TF is restored when the upgrade is aborted (use the mock for that?) and also not called when we don't abort?
There was a problem hiding this comment.
I don't have a strong opinion on mocks, but I think we should discuss if we want to actively use mock testing in the team and either do it in every package when someone edits it
There was a problem hiding this comment.
I agree that mocking is a last resort. Of course it would be more resilient to only mock the filesystem and actually check that the files are restored
There was a problem hiding this comment.
I do see the point in using mocks for verifying that the expected code path is taken here. However, I don't think that this fits very well with our current testing strategy here, as it would require us to encode logic in our tests, which is often recognized as an anti-pattern. (see here, 12-14 to 12-16) I do see the values coming from these tests, but I think if we decide to adopt this, we should do so as a whole. I think we should avoid mocking only in certain tests, to keep some consistency. I might have a wrong understanding of how to use these mocks too. If you see a way to use it without putting too much logic into the test, feel free to let me know.
| }, | ||
| yesFlag: true, | ||
| wantErr: true, | ||
| }, |
There was a problem hiding this comment.
thanks for adding the tests. could we add the restore called check here too?
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Context
Previously, on an upgrade including Terraform changes, Constellation would create an external folder
./constellation-upgrade/abc/terraformwith a copy of the pre-upgrade Terraform state and the post-upgrade wanted Terraform configuration, and do aterraform planandterraform applythere. Only if the upgrade would complete successfully, the folder would be copied from./constellation-upgrade/abc/terraforminto./constellation-terraform. If an upgrade failed, the intermediate state would be lost and users weren't able to useterraform importon the intermediate state.Proposed change(s)
./constellation-upgrade/abc/terraform-backupor./constellation-upgrade/abc/terraform-iam-backupconstellation-terraformorconstellation-iam-terraformfolder.Additional info
Checklist