SALTO-218: apply changes from state to bps post deploy #343

hadard · 2019-11-04T15:09:26Z

No description provided.

adamro · 2019-11-04T22:03:38Z

packages/cli/src/commands/deploy.ts

      getConfigFromUser,
      shouldDeploy({ stdout: this.stdout, stderr: this.stderr }),
      (action: PlanItem) => this.updateCurrentAction(action), this.force)
    this.endCurrentAction()
-    return CliExitCode.Success
+    if (result.changes) {


Let's sync tomorrow about how this works with my changes

geymed · 2019-11-04T22:13:22Z

packages/cli/src/callbacks.ts

+  return getApprovedChanges(changes, formatFetchChangeForApproval, isConflict)
+}
+
+export const getApprovedDetailedChanges = async (


Maybe getApprovedDeployChanges for consistency?

ori-moisis · 2019-11-05T07:57:00Z

packages/cli/src/callbacks.ts

-    message: formatFetchChangeForApproval(change, idx, changes.length),
-    when: answers => isConflict(change) || !shouldDeployAll(answers),
+    message: message(change, idx, changes.length),
+    when: answers => (when ? when(change) : false) || !shouldDeployAll(answers),


not related to this PR but shouldDeployAll should not be called deploy, should probably be called shouldApproveAll
Also, allowing the function when to come from an external source can cause confusion IMO because its decision is sometimes overridden by logic that is internal to this function.

Generally I think there is no reason to have different behavior for changes originating from apply and changes originating from fetch, IMO they can use the same types and the same flow and I think that would make the code much simpler

not sure how we can do it, in fetch flow we have FetchChange and in deploy flow we have diff between state and workspace which is only DetailedChange. Do you suggest to build in deploy flow fake FetchChange?

ori-moisis · 2019-11-05T08:14:44Z

packages/cli/src/commands/deploy.ts

      getConfigFromUser,
      shouldDeploy({ stdout: this.stdout, stderr: this.stderr }),
      (action: PlanItem) => this.updateCurrentAction(action), this.force)
    this.endCurrentAction()
-    return CliExitCode.Success
+    if (result.changes) {
+      const changes = wu(result.changes).toArray()


style nit: no need to use wu when just filling an array, can be written as [...result.changes]

ori-moisis · 2019-11-05T08:19:21Z

packages/cli/src/formatter.ts

+    header(Prompts.FETCH_CHANGE_HEADER(idx + 1, totalChanges)),
+    body(formatDetailedChanges([[change]])),
+    header(Prompts.FETCH_SHOULD_APPROVE_CHANGE),
+  ].join('\n'))


a change with no conflict behaves exactly the same (and we really want to the be exactly the same for UX consistency) so this code duplication is not a good idea IMO

ori-moisis · 2019-11-05T08:22:27Z

packages/cli/test/mocks.ts

-export const importFromCsvFile = async (): Promise<void> => {}
-export const deleteFromCsvFile = async (): Promise<void> => {}
+export const importFromCsvFile = async (): Promise<void> => { }
+export const deleteFromCsvFile = async (): Promise<void> => { }


nit: why add the space here?

😄 vscode added it

ori-moisis · 2019-11-05T08:23:18Z

packages/salto-vscode/src/commands.ts

@@ -105,13 +105,12 @@ export const deployCommand = async (
  }

  try {
-    deployProcess = deploy(
+    await deploy(


cli and vscode should behave consistently, if the cli command updates the workspace, so should vscode

you right opened: SALTO-296

ori-moisis · 2019-11-05T08:25:17Z

packages/salto/src/api.ts

 export const preview = async (
  workspace: Workspace,
 ): Promise<Plan> => {
  const state = new State(workspace.config.stateLocation)
  return getPlan(await state.get(), workspace.elements)
 }

+export interface DeployResult {
+  sucesses: boolean
+  changes?: Iterable<DetailedChange>


I think we should prefer returning an empty list to having changes optional here. optional fields are annoying to handle in the code and when using lists it means there are actually two different ways for express no changes (undefined and empty list)

the optional is for the case of success=false not sure what's better..

ori-moisis · 2019-11-05T08:29:07Z

packages/salto/src/api.ts

@@ -56,8 +57,12 @@ export const deploy = async (
        reportProgress,
        (action, element) => deployActionOnState(state, action, element)
      )
+      return {
+        sucesses: true,
+        changes: getDetailedChanges(workspace.elements, await state.get()) || undefined,


why do we need || undefined here? doesn't really make sense given that getDetailedChanges should always return an iterable

I wasn't sure how this is going to behave - you probably right and I will remove it

ori-moisis · 2019-11-05T13:59:47Z

packages/cli/src/callbacks.ts

  changes: ReadonlyArray<FetchChange>,
 ): Promise<ReadonlyArray<FetchChange>> => {
+  const isConflict = (change: FetchChange): boolean => change.pendingChange !== undefined


please revert this one so we can avoid conflict

ori-moisis · 2019-11-05T14:06:00Z

packages/cli/test/mocks.ts

+    action: 'remove',
+    data: { before: 'asd' },
+  },
+]


if this is already here we can use the detailedChange function

ori-moisis · 2019-11-05T14:09:24Z

packages/salto/src/api.ts

@@ -56,8 +57,15 @@ export const deploy = async (
        reportProgress,
        (action, element) => deployActionOnState(state, action, element)
      )
+
+      const changes = wu(getDetailedChanges(workspace.elements, await state.get()))


probably better not to state.get() again here, instead I would suggest storing the state elements and leaving a comment saying they are mutated during deploy and we rely on this

* Added settings filter * End help message with EOL (#340) * removed extension readme badges (#345) * fixes command title (#339) * added event listener to file changes in extension (#338) * added error severity in extension (#344) * added error severity in extension * changed event filter out non-bp file changes (#341) * added wrapper to run aggr set function (fixes SALTO-264) (#342) * try #1 to fix Capriza failure: "TypeError: Cannot read property 'lookupFilter' of undefined" (#346) * SALTO-218: apply changes from state to bps post deploy (#343) * Replaced async-file package (#347) * upgraded inquirer.js to fix hang in Windows 10 * moved accesses to fs module to centralized file module * using tmp-promise instead of tmp * change default fetch behavior to auto approve incoming changes (#350) * Added settings filter * Hadar CR * Consistent list order (#348) List ordering for specific ObjectTypes and records (InstanceElements) * missing await in apply changes post deploy (#356) * added missing await in fetch (#353) * Consistent list order (#348) List ordering for specific ObjectTypes and records (InstanceElements) * missing await in apply changes post deploy (#356) * Added settings filter * Hadar CR * add debug logs (#355) * blacklisted static resources (#352) * shouldn't add default ObjectType annotations upon adapter.update (#357) * Hadar comments * rename elemId fieldType name to field_name since it collides with a Custom object that named Name and it causes undesired merge of the ObjectTypes (#354) * Fixed e2e * Fixed e2e * Added settings filter * Hadar CR * Hadar comments * Fixed e2e * Fixed e2e

hadard requested a review from a team November 4, 2019 15:36

SALTO-218: apply changes from state to bps post deploy

fd69192

hadard force-pushed the apply_changes_post_deploy branch from f568d82 to fd69192 Compare November 4, 2019 20:25

merge local leftovers

7bbb998

adamro reviewed Nov 4, 2019

View reviewed changes

geymed reviewed Nov 4, 2019

View reviewed changes

CR fix - rename getApprovedDetailedChanges -> getApprovedDeployChanges

407982d

ori-moisis reviewed Nov 5, 2019

View reviewed changes

hadard added 2 commits November 5, 2019 12:13

CR: fix progress of deploy in adapter

c229b2e

CR fixes - return FetchChange instead of DetailedChange

266c6e8

ori-moisis approved these changes Nov 5, 2019

View reviewed changes

more CR

411ca61

hadard merged commit 5762728 into salto-io:master Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SALTO-218: apply changes from state to bps post deploy #343

SALTO-218: apply changes from state to bps post deploy #343

hadard commented Nov 4, 2019

adamro Nov 4, 2019

hadard Nov 5, 2019

geymed Nov 4, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

ori-moisis Nov 5, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

hadard Nov 5, 2019

ori-moisis Nov 5, 2019

SALTO-218: apply changes from state to bps post deploy #343

SALTO-218: apply changes from state to bps post deploy #343

Conversation

hadard commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment