[Feat] Show target instance before deploy, stop, destroy and compact - #64
[Feat] Show target instance before deploy, stop, destroy and compact#64bean1352 wants to merge 6 commits into
Conversation
LucDeCaf
left a comment
There was a problem hiding this comment.
I like this idea, looks good 👍
| const validationTestsFilter = GENERAL_VALIDATION_FLAG_HELPERS.parseValidationTestFlags(flags); | ||
|
|
||
| const cloudConfigState = await this.loadCloudConfigState(); | ||
| await this.logTargetInstance({ instanceName: cloudConfigState.name }); |
There was a problem hiding this comment.
It's nice to be logging the current instance here, but, at this point - is it not too late if there is a mistake? It looks like the deploy will continue after logging the instance.
Perhaps we could add a dedicated command to print the resolved instance (if using env vars or cli.yaml). Or add some optional interaction to these commands.
There was a problem hiding this comment.
Maybe an optional flag like --dry or --checked to halt / pause execution and request the user to confirm the action:
$ powersync deploy --instance-id="6a98319739369b69bd8fc57f" --checked
Found instance: production (id "6a98319739369b69bd8fc57f")
> Confirm "deploy" action for instance "production"? (y/N) n
Operation canceled.
This could also be the default behaviour with an --unchecked flag instead, but that would obviously need a breaking release:
$ powersync deploy --instance-id="6a98319739369b69bd8fc57f"
Found instance: production (id "6a98319739369b69bd8fc57f")
> Confirm "deploy" action for instance "production"? (y/N) y
Deployed instance "production" with ID "6a98319739369b69bd8fc57f".
======
$ powersync deploy --instance-id="6a98319739369b69bd8fc57f" --unchecked
> Warning: Using `--unchecked` with potentially dangerous action (deploy).
Found instance: production (id "6a98319739369b69bd8fc57f")
Deployed instance "production" with ID "6a98319739369b69bd8fc57f".
There was a problem hiding this comment.
I like the --dry-run like option. We could even perhaps show a basic diff of the config changes which would be deployed.
There was a problem hiding this comment.
I like this idea but wouldn't this be a breaking change? Existing CI workflows would just hang if the cli is upgraded.
There was a problem hiding this comment.
The --dry-run flag would only change behaviour if a dev explicitly added it to the command invocation - so that one would not be a breaking change.
Adding optional interaction, should also only be opt-in, by specifying some flag (like the --checked option mentioned above).
The --unchecked example is a breaking change.
I'm more in favour of the --dry-run option, since it's not breaking and could actually be useful if we showed a diff.
LucDeCaf
left a comment
There was a problem hiding this comment.
Tested this on my side, looks good
What
Prints the resolved instance name and IDs before
deploy,deploy sync-config,deploy service-config,stop,destroyandcompactdo anything, so you can see which instance is about to change.statusprints the target first as well, with the API URL for self-hosted instances. JSON and YAML output are unchanged.deployanddeploy service-configalso warn whenservice.yamlhas a differentnamethan the instance, because deploying renames it.Dry run
All deploy commands accept
--dry-run. It prints the target instance, runs the validations, and stops before deploying or provisioning. It also shows what would change:When the instance is deprovisioned, a real deploy provisions it first. The dry run says so and skips the sync config validation that needs a running instance.
stopanddestroyalready require--confirm=yes, so they are unchanged.Notes
diff(jsdiff) as a direct dependency of the CLI. It was already in the lockfile as a transitive dependency.secret: !env ...always showreplicationas changed, because the value is sent again. This is documented in the usage docs.AI disclosure
I used Claude to research the codebase and implement the change. I reviewed and tested it myself.