frodo config import --compare-and-delete , --dry-run flag #24
Conversation
…fig data from cloud into memory as an object and compares that with master file/directory, and deletes the differences, and imports master file/directory back to cloud.
skootrivir
force-pushed
the
config-import-compareAndDelete
branch
from
a670836 to
264a01d
Compare
phalestrivir
left a comment
phalestrivir
left a comment
There was a problem hiding this comment.
Make sure you rename everything to not use ‘cloud’, since in the future we may want to do this same functionality for other deployments like on-prem. We should probably discuss with @hfranklin if this is something we want to do now or save for later. If we save it for later, we should probably update the getTokens method in [config-import.ts](http://config-import.ts/) to only allow cloud deployments when doing --compare.
I'm also wondering if, since we are doing similar deleting and importing as what we are doing for PromoteOps, I think we should use the same function for deleting and importing in both places to reduce code redundancy and ensure that both PromoteOps and the compare delete for ConfigOps work the same way.
src/cli/config/config-import.ts
Outdated
| 'Import all scripts including the default scripts.' | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Remove this extra new line since it wasn't there before
src/cli/config/config-import.ts
Outdated
| ) | ||
| .addOption( | ||
| new Option( | ||
| '--delete-and-import', |
There was a problem hiding this comment.
I was wondering if we should rename this command to something more concise, just as --sync or --reconcile, or maybe --sync-compare or --reconcile-compare so it's obvious that the flag is related to the --compare flag. It's probably fine to keep it as --delete-and-import for now though, but we may want to ask @hfranklin what he thinks makes the most sense here.
src/cli/config/config-import.ts
Outdated
| .addOption( | ||
| new Option( | ||
| '--delete-and-import', | ||
| 'Deletes whatever added to the current cloud after comparison from --compare flag. Then it imports master config file/directory back to cloud. Only run when --compare flag is on.' |
There was a problem hiding this comment.
This description is a little confusing to me. I think this might be a better description:
'Syncs changes found by using --compare from the master config file/directory to the current deployment. This includes importing any new/updated configuration, and deleting any configuration that is not in the master config.'
src/cli/config/config-import.ts
Outdated
| .addOption( | ||
| new Option( | ||
| '--compare', | ||
| 'This export config data from the current tenant as an object, and it compares whatever changes between this object and the config data from the master file/directory.' |
There was a problem hiding this comment.
This description can be more concise. How about this:
'Compares changes between config in the master file or directory (specified with -f or -D flags respectively) with config exported from the current deployment.'
We will also want to also update the descriptions for -f and -D to mention that they are also used to specify the location for the master config for the --compare flag.
src/ops/ConfigOps.ts
Outdated
| /** | ||
| * Export from current tenant, compare with master directory, delete the differences and import master back | ||
| */ | ||
| export async function compareWithMasterDirectoryAndDeleteFromCloud( |
There was a problem hiding this comment.
This function is very similar to compareWithMasterFileAndDeleteFromCloud. I would put all the same code from both into a helper function to reduce code redundancy.
| await deleteServiceNextDescendentOnly(serviceId, globalConfig); | ||
| return true; | ||
| } catch (error) { | ||
| printError(error); |
There was a problem hiding this comment.
Should return false in the event of an error
| -D. Ignored with -i or -a. | ||
| -C, --clean Remove existing service(s) before | ||
| importing. | ||
| --compare-and-delete Export current cloud config data into |
There was a problem hiding this comment.
Snapshots need to be updated with new flags.
| " | ||
| `; | ||
|
|
||
| exports[`frodo config import "frodo config import -AD test/e2e/exports/all-separate/cloud --compare-and-delete --include-active-values" Import everything with secret values from directory "test/e2e/exports/all-separate/cloud" 1`] = ` |
There was a problem hiding this comment.
This test failed, as it printed out the help menu. You'll want to debug and fix this. Same for anywhere else this happens
test/e2e/config-import.e2e.test.js
Outdated
| await exec(CMD, env); | ||
| fail("Command should've failed") | ||
| await exec(CMD, env); | ||
| fail("Command should've failed") |
There was a problem hiding this comment.
Revert these spacing changes, including all those in the rest of the file
2 participants
frodo config import ... --compare-and-delete command:
frodo config import ... --compare-and-delete --dry-run command:
1.Exports config data from cloud as an object
2. compares that object with master file/directory and saves comparing results in .txt file (log from export command blocks the result to be shown on terminal, so the result should be saved in local directory.
--dry-run can only be used with --compare-and-delete flag.
--dry-run let --compare-and-delete flag to only export the cloud and compare it with the master.
--dry-run will prevent --compare-and-delete flag to delete and import