-
Notifications
You must be signed in to change notification settings - Fork 2
[Feat] Show target instance before deploy, stop, destroy and compact #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
a593c1b
show target instance before deploy, stop, destroy and compact
bean1352 ebb051d
handle windows warning marker in deploy test
bean1352 37de2de
add --dry-run with config diff to deploy commands
bean1352 2f2d34e
show target instance in status
bean1352 a3037c8
tidy target labels and dry-run notes
bean1352 e54b0cd
simplify target banner and dry-run summary
bean1352 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| '@powersync/cli-core': patch | ||
| 'powersync': patch | ||
| --- | ||
|
|
||
| Show the target instance name and IDs before `deploy`, `deploy sync-config`, `deploy service-config`, `stop`, `destroy` and `compact` do anything, so it is clear which instance is about to be changed. `status` shows the target first as well, with the API URL for self-hosted instances. `deploy` and `deploy service-config` now also warn when the local `service.yaml` `name` differs from the instance name, since deploying renames the instance. | ||
|
|
||
| All deploy commands accept `--dry-run`, which prints the target instance, runs the validations, shows a diff of the sync config and the changed service config sections, and stops without deploying. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { ux } from '@oclif/core'; | ||
| import { AdditionalCloudConfigFields, ServiceCloudConfigDecoded } from '@powersync/cli-schemas'; | ||
| import { routes } from '@powersync/management-types'; | ||
| import { structuredPatch } from 'diff'; | ||
| import isEqual from 'lodash/isEqual.js'; | ||
|
|
||
| import { decodeFetchedCloudConfig } from './cloud/fetch-cloud-config.js'; | ||
|
|
||
| const CLI_ONLY_FIELDS = new Set(Object.keys(AdditionalCloudConfigFields.props.shape)); | ||
|
|
||
| function colorizeDiffLine(line: string): string { | ||
| if (line.startsWith('+')) return ux.colorize('green', line); | ||
| if (line.startsWith('-')) return ux.colorize('red', line); | ||
| return line; | ||
| } | ||
|
|
||
| /** | ||
| * Names the top-level service config sections whose local value differs from the deployed one. | ||
| * Returns undefined when the deployed config cannot be decoded for comparison. | ||
| */ | ||
| export function changedServiceConfigSections( | ||
| localConfig: ServiceCloudConfigDecoded, | ||
| cloudConfigState: routes.InstanceConfigResponse | ||
| ): string[] | undefined { | ||
| let deployed: Record<string, unknown>; | ||
| try { | ||
| deployed = decodeFetchedCloudConfig(cloudConfigState).config as Record<string, unknown>; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
|
|
||
| const local = localConfig as Record<string, unknown>; | ||
| const sections = new Set([...Object.keys(deployed), ...Object.keys(local)]); | ||
| return [...sections] | ||
| .filter((section) => !CLI_ONLY_FIELDS.has(section) && !isEqual(local[section], deployed[section])) | ||
| .sort(); | ||
| } | ||
|
|
||
| /** Unified diff of the deployed sync config against the local one, one colorized entry per line. Empty when identical. */ | ||
| export function formatSyncConfigDiff(deployed: string, local: string): string[] { | ||
| const { hunks } = structuredPatch('deployed', 'local', deployed, local); | ||
| return hunks.flatMap((hunk) => [ | ||
| ux.colorize('cyan', `@@ -${hunk.oldStart},${hunk.oldLines} +${hunk.newStart},${hunk.newLines} @@`), | ||
| ...hunk.lines.map((line) => colorizeDiffLine(line)) | ||
| ]); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an optional flag like
--dryor--checkedto halt / pause execution and request the user to confirm the action:This could also be the default behaviour with an
--uncheckedflag instead, but that would obviously need a breaking release:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the
--dry-runlike option. We could even perhaps show a basic diff of the config changes which would be deployed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
--dry-runflag 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
--checkedoption mentioned above).The
--uncheckedexample is a breaking change.I'm more in favour of the
--dry-runoption, since it's not breaking and could actually be useful if we showed a diff.