cli: dry-run Terraform migrations on upgrade check
#1942
Conversation
|
Could you please add the Context section of the new PR template to the description? |
|
I would suggest adding a flag This would also solve this problem?
|
| @@ -120,8 +120,7 @@ func (u *TerraformUpgrader) PlanTerraformMigrations(ctx context.Context, opts Te | |||
| // aborted by the user. | |||
| func (u *TerraformUpgrader) CleanUpTerraformMigrations(fileHandler file.Handler, upgradeID string) error { | |||
| cleanupFiles := []string{ | |||
| filepath.Join(constants.UpgradeDir, upgradeID, constants.TerraformUpgradeBackupDir), | |||
| filepath.Join(constants.UpgradeDir, upgradeID, constants.TerraformUpgradeWorkingDir), | |||
| filepath.Join(constants.UpgradeDir, upgradeID), | |||
There was a problem hiding this comment.
is it safe to not specify the subdirs anymore? not saying it is not, but just want to raise potential implications if there could be more directories. We still seem to follow explicit listing in l.97f
There was a problem hiding this comment.
Yeah, I should've changed this when we implemented the upgrade versioning. Before we had the 2 subfolders that needed to be cleaned individually. Now we have 1 folder {constants.UpgradeDir}/{upgradeID}
| writeConfig bool | ||
| ref string | ||
| stream string | ||
| terraformLogLevel terraform.LogLevel | ||
| } | ||
|
|
||
| type upgradeChecker interface { |
There was a problem hiding this comment.
the interface seems to go away from what the name suggests: "checking" and instead has side-effects.
PlanTerraformMigrations and CleanUpTerraformMigrations don't seem to belong there.
By seperating this we could maybe also avoid passing the same checker object twice in runUpgradeCheck for cmd creation?
There was a problem hiding this comment.
we won't come around passing the checker twice as it's creating all resources needed for upgrades (i.e. also the Terraform client).
However I do agree that the naming and separation can be changed.
|
Happy to jump on a call to discuss together |
We should see what @derpsteb thinks as he initially designed the upgrade flow |
a10009f to
92b192c
Compare
92b192c to
92907b6
Compare
92907b6 to
44357a7
Compare
|
We had a short discussion on the |
* upgrade versioning * dont pass upgrade kind as boolean * whitespace * fix godot lint check * clarify upgrade check directory suffix * cli: dry-run Terraform migrations on `upgrade check` (#1942) * dry-run Terraform migrations on upgrade check * clean whole upgrade dir * clean up check workspace after planning * fix parsing * extend upgrade check test * rename unused parameters * exclude false positives in test
Context
upgrade checkcommand used not to take the Terraform migrations into account that would happen within the checked upgrade. This should be changed in this PR.Proposed change(s)
upgrade checkcommand should perform a dry-run of the Terraform migrations that will be applied within the update, to show the user which actions on cloud resources will be taken upon the update.Additional info
parseTerraformUpgradeVarsfunction is shared between both theupgrade checkand theupgrade applycommand. This is certainly not a great dependency model and it should likely be factored out. I was unsure about what an appropriate place might be.Checklist