cli: store upgrade files in versioned folders #1929
Conversation
✅ Deploy Preview for constellation-docs canceled.
|
cli/internal/kubernetes/upgrade.go
Outdated
| // If checkOnly is true, the upgrader will only create the resources necessary for upgrade checking. (i.e. no Terraform client) | ||
| // Not creating the Terraform client in the check-case makes sense as the initialization of the Terraform client copies the | ||
| // executable to the upgrade directory, which is not necessary for the check-only case. | ||
| func NewUpgrader(ctx context.Context, outWriter io.Writer, log debugLog, checkOnly bool) (*Upgrader, error) { |
There was a problem hiding this comment.
I agree that we might not always want to initialize the terraform client here. But if we don't, we should guard all access to tfUpgrade with a null-pointer check. if u.tfUpgrade == nil {...}. Alternatively you could omit the flag, put the initialization into a function and call that init function in case u.tfUpgrade is nil.
One could also argue that our upgrade check is incomplete, as it does not check if there are migrations that would need to be applied. If I am not mistaken we are applying migrations during upgrade apply unconditionally. Meaning that it makes no difference if the user is only upgrading services, image or k8s - migrations are applied in any case. Because of that, upgrade check could do a dry-run terraform apply to inform the user about the upcoming migration. I don't expect that you do this right now. I just had this thought and would like to hear what others think.
There was a problem hiding this comment.
re. upgrade check:
We could certainly do this! However I think we should not add the "cloud resources" as a singular upgradable component such as microservices, k8s, etc. - Since the CLI gets upgraded either way, it might expect the cloud resources to exist no matter which component got upgraded. I think both upgrade check and upgrade apply should always consider the Terraform migrations.
There was a problem hiding this comment.
re. nil check:
The main reason for not initializing the Terraform client here was that in the case of an upgrade check, the Terraform upgrade directories were created as well, as they get initialized with the Terraform client. If we dry-run the migrations in upgrade check, this would be obsolete.
There was a problem hiding this comment.
As it probably needs some more code changes which are out of scope for this PR, I track this in AB#3226.
|
Looks great. Just that one comment. Started another e2e testrun as I think the one you posted didn't upgrade the services and thus didn't execute the changed code. I am removing the |
|
See #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
|
If you could update the PR title to something slightly more descriptive. That would be great. E.g. "cli: store backups during upgrade in timestamped folders". Thanks! |
msanft
changed the title
Proposed change(s)
constellation-upgradedirectory. Each upgrade is marked by a timestamp string and a unique identifier and the corresponding files get placed into a unique subdirectory for their upgrade ID. (e.g.constellation-upgrade/upgrade-20230614105831-976b92d0)Additional info
Checklist