Merge modifications made in previous versions when upgrading

dashboard/package.json

@@ -22,6 +22,7 @@
     "axios": "^0.19.0",
     "connected-react-router": "^4.5.0",
     "enzyme-adapter-react-16": "^1.1.1",
+    "fast-json-patch": "^3.0.0-1",
     "fstream": "^1.0.12",
     "js-yaml": "^3.13.1",
     "json-schema": "^0.2.5",

dashboard/src/components/DeploymentFormBody/DeploymentFormBody.test.tsx

@@ -29,7 +29,7 @@ const defaultProps = {
   valuesModified: false,
   setValues: jest.fn(),
   setValuesModified: jest.fn(),
-  originalValues: undefined,
+  deployedValues: undefined,
 } as IDeploymentFormBodyProps;
 const versions = [{ id: "foo", attributes: { version: "1.2.3" } }] as IChartVersion[];
 
@@ -118,9 +118,9 @@ describe("when there are changes in the selected version", () => {
   describe("when the user has not modified any value", () => {
     it("selects the original values if the version doesn't change", () => {
       const setValues = jest.fn();
-      const originalValues = "foo: notBar";
+      const deployedValues = "foo: notBar";
       const wrapper = shallow(
-        <DeploymentFormBody {...props} setValues={setValues} originalValues={originalValues} />,
+        <DeploymentFormBody {...props} setValues={setValues} deployedValues={deployedValues} />,
       );
       wrapper.setProps({
         selected: {
@@ -139,13 +139,13 @@ describe("when there are changes in the selected version", () => {
       const localState: IDeploymentFormBodyState = wrapper.instance()
         .state as IDeploymentFormBodyState;
       expect(localState.basicFormParameters).toEqual(basicFormParameters);
-      expect(setValues).toHaveBeenCalledWith("foo: notBar");
+      expect(setValues).toHaveBeenCalledWith("foo: notBar\n");
     });
 
     it("uses the chart default values when original values are not defined", () => {
       const setValues = jest.fn();
       const wrapper = shallow(
-        <DeploymentFormBody {...props} setValues={setValues} originalValues={undefined} />,
+        <DeploymentFormBody {...props} setValues={setValues} deployedValues={undefined} />,
       );
       wrapper.setProps({
         selected: {
@@ -171,13 +171,13 @@ describe("when there are changes in the selected version", () => {
   describe("when the user has modified the values", () => {
     it("will ignore original or default values", () => {
       const setValues = jest.fn();
-      const originalValues = "foo: ignored-value";
+      const deployedValues = "foo: ignored-value";
       const modifiedValues = "foo: notBar";
       const wrapper = shallow(
         <DeploymentFormBody
           {...props}
           setValues={setValues}
-          originalValues={originalValues}
+          deployedValues={deployedValues}
           valuesModified={true}
           appValues={modifiedValues}
         />,
@@ -382,3 +382,106 @@ it("restores the default chart values when clicking on the button", () => {
 
   expect(setValues).toHaveBeenCalledWith("foo: value");
 });
+
+[
+  {
+    description: "should merge modifications from the values and the new version defaults",
+    defaultValues: "foo: bar\n",
+    deployedValues: "foo: bar\nmy: var\n",
+    newDefaultValues: "notFoo: bar",
+    result: "notFoo: bar\nmy: var\n",
+  },
+  {
+    description: "should modify the default values",
+    defaultValues: "foo: bar\n",
+    deployedValues: "foo: BAR\nmy: var\n",
+    newDefaultValues: "foo: bar",
+    result: "foo: BAR\nmy: var\n",
+  },
+  {
+    description: "should delete an element in the defaults",
+    defaultValues: "foo: bar\n",
+    deployedValues: "my: var\n",
+    newDefaultValues: "foo: bar\n",
+    result: "my: var\n",
+  },
+  {
+    description: "should add an element in an array",
+    defaultValues: `foo:
+  - foo1:
+    bar1: value1
+`,
+    deployedValues: `foo:
+  - foo1: 
+    bar1: value1
+  - foo2: 
+    bar2: value2
+`,
+    newDefaultValues: `foo:
+    - foo1:
+      bar1: value1
+`,
+    result: `foo:
+  - foo1: 
+    bar1: value1
+  - foo2: 
+    bar2: value2
+`,
+  },
+  {
+    description: "should delete an element in an array",
+    defaultValues: `foo:
+  - foo1:
+    bar1: value1
+  - foo2:
+    bar2: value2
+`,
+    deployedValues: `foo:
+  - foo1: 
+    bar1: value1
+`,
+    newDefaultValues: `foo:
+  - foo1:
+    bar1: value1
+  - foo2:
+    bar2: value2
+`,
+    result: `foo:
+  - foo1: 
+    bar1: value1
+`,
+  },
+].forEach(t => {
+  it(t.description, () => {
+    const selected = {
+      ...defaultProps.selected,
+      versions: [chartVersion],
+      version: chartVersion,
+      values: t.defaultValues,
+      schema: initialSchema,
+    };
+    const newSelected = {
+      ...defaultProps.selected,
+      versions: [chartVersion],
+      version: chartVersion,
+      values: t.newDefaultValues,
+      schema: initialSchema,
+    };
+    const setValues = jest.fn();
+    const wrapper = shallow(
+      <DeploymentFormBody
+        {...props}
+        deployedValues={t.deployedValues}
+        setValues={setValues}
+        selected={{ versions: [] }}
+      />,
+    );
+    // Store the modifications
+    wrapper.setProps({ selected });
+    expect(setValues).toHaveBeenCalledWith(t.deployedValues);
+
+    // Apply new version
+    wrapper.setProps({ selected: newSelected });
+    expect(setValues).toHaveBeenCalledWith(t.result);
+  });
+});

dashboard/src/components/DeploymentFormBody/DeploymentFormBody.tsx

@@ -1,8 +1,10 @@
 import { RouterAction } from "connected-react-router";
+import * as jsonpatch from "fast-json-patch";
 import * as React from "react";
 import { Tab, TabList, TabPanel, Tabs } from "react-tabs";
+import * as YAML from "yaml";
 
-import { retrieveBasicFormParams, setValue } from "../../shared/schema";
+import { deleteValue, retrieveBasicFormParams, setValue } from "../../shared/schema";
 import { IBasicFormParam, IChartState } from "../../shared/types";
 import { getValueFromEvent } from "../../shared/utils";
 import ConfirmDialog from "../ConfirmDialog";
@@ -18,7 +20,7 @@ import "./Tabs.css";
 export interface IDeploymentFormBodyProps {
   chartID: string;
   chartVersion: string;
-  originalValues?: string;
+  deployedValues?: string;
   namespace: string;
   releaseName?: string;
   selected: IChartState["selected"];
@@ -35,6 +37,7 @@ export interface IDeploymentFormBodyProps {
 export interface IDeploymentFormBodyState {
   basicFormParameters: IBasicFormParam[];
   restoreDefaultValuesModalIsOpen: boolean;
+  modifications?: jsonpatch.Operation[];
 }
 
 class DeploymentFormBody extends React.Component<
@@ -64,18 +67,37 @@ class DeploymentFormBody extends React.Component<
     if (nextProps.selected !== this.props.selected) {
       // The values or the schema has changed
       let values = "";
+      // Get the original modification to the values if exists
+      let modifications = this.state.modifications;
+      // TODO(andresmgot): Right now, are taking advantage of the fact that when first
+      // loaded this component (in the upgrade scenario) the "selected" version is the
+      // currently deployed version. We should change that to be the latest version available
+      // so the current approach won't be possible. We should also try to avoid to modify
+      // the behavior of this component depending on the scenario (install/upgrade).
+      if (nextProps.selected.values && this.props.deployedValues && !modifications) {
+        const defaultValuesObj = YAML.parse(nextProps.selected.values);
+        const deployedValuesObj = YAML.parse(this.props.deployedValues);
+        modifications = jsonpatch.compare(defaultValuesObj, deployedValuesObj);
+        this.setState({ modifications });
+      }
+
       if (!this.props.valuesModified) {
-        // If the version is the current one, reuse original params
-        // (this only applies to the upgrade form that has originalValues defined)
-        if (
-          nextProps.selected.version &&
-          nextProps.selected.version.attributes.version === this.props.chartVersion &&
-          this.props.originalValues
-        ) {
-          values = this.props.originalValues || "";
-        } else {
-          // In other case, use the default values for the selected version
-          values = nextProps.selected.values || "";
+        // In other case, use the default values for the selected version
+        values = nextProps.selected.values || "";
+        // And we add any possible change made to the original version
+        if (modifications) {
+          modifications.forEach(modification => {
+            // Transform the JSON Path to the format expected by setValue
+            // /a/b/c => a.b.c
+            const path = modification.path.replace(/^\//, "").replace(/\//g, ".");
+            if (modification.op === "remove") {
+              values = deleteValue(values, path);
+            } else {
+              // Transform the modification as a ReplaceOperation to read its value
+              const value = (modification as jsonpatch.ReplaceOperation<any>).value;
+              values = setValue(values, path, value);
+            }
+          });
         }
         this.props.setValues(values);
       } else {

dashboard/src/components/UpgradeForm/UpgradeForm.tsx

@@ -60,7 +60,7 @@ class UpgradeForm extends React.Component<IUpgradeFormProps, IUpgradeFormState>
             <DeploymentFormBody
               chartID={chartID}
               chartVersion={this.props.appCurrentVersion}
-              originalValues={this.props.appCurrentValues}
+              deployedValues={this.props.appCurrentValues}
               namespace={this.props.namespace}
               releaseName={this.props.releaseName}
               selected={this.props.selected}

dashboard/src/components/UpgradeForm/__snapshots__/UpgradeForm.test.tsx.snap

@@ -22,11 +22,11 @@ exports[`renders the full UpgradeForm 1`] = `
         appValues="foo: bar"
         chartID="my-repo/my-chart"
         chartVersion="1.0.0"
+        deployedValues="foo: bar"
         fetchChartVersions={[Function]}
         getChartVersion={[Function]}
         goBack={[Function]}
         namespace="default"
-        originalValues="foo: bar"
         push={[Function]}
         releaseName="my-release"
         selected={

dashboard/src/shared/schema.test.ts

@@ -1,6 +1,6 @@
 import { JSONSchema4 } from "json-schema";
 
-import { getValue, retrieveBasicFormParams, setValue, validate } from "./schema";
+import { deleteValue, getValue, retrieveBasicFormParams, setValue, validate } from "./schema";
 import { IBasicFormParam } from "./types";
 
 describe("retrieveBasicFormParams", () => {
@@ -315,6 +315,14 @@ describe("setValue", () => {
       result: "foo: bar\nthis: { new: { value: 1 } }\n",
       error: false,
     },
+    {
+      description: "Adding a value in an empty doc",
+      values: "",
+      path: "foo",
+      newValue: "bar",
+      result: "foo: bar\n",
+      error: false,
+    },
   ].forEach(t => {
     it(t.description, () => {
       let res: any;
@@ -328,6 +336,44 @@ describe("setValue", () => {
   });
 });
 
+describe("deleteValue", () => {
+  [
+    {
+      description: "should delete a value",
+      values: "foo: bar\nbar: foo\n",
+      path: "bar",
+      result: "foo: bar\n",
+    },
+    {
+      description: "should delete a value from an array",
+      values: `foo:
+  - bar
+  - foobar
+`,
+      path: "foo.0",
+      result: `foo:
+  - foobar
+`,
+    },
+    {
+      description: "should leave the document emtpy",
+      values: "foo: bar",
+      path: "foo",
+      result: "\n",
+    },
+    {
+      description: "noop when trying to delete a missing property",
+      values: "foo: bar\nbar: foo\n",
+      path: "var",
+      result: "foo: bar\nbar: foo\n",
+    },
+  ].forEach(t => {
+    it(t.description, () => {
+      expect(deleteValue(t.values, t.path)).toEqual(t.result);
+    });
+  });
+});
+
 describe("validate", () => {
   [
     {

dashboard/src/shared/schema.ts

@@ -3,7 +3,7 @@
 // that are used in this package
 import * as AJV from "ajv";
 import * as jsonSchema from "json-schema";
-import { set } from "lodash";
+import { isEmpty, set } from "lodash";
 import * as YAML from "yaml";
 import { IBasicFormParam } from "./types";
 
@@ -73,9 +73,11 @@ function getDefinedPath(allElementsButTheLast: string[], doc: YAML.ast.Document)
   return currentPath;
 }
 
-// setValue modifies the current values (text) based on a path
-export function setValue(values: string, path: string, newValue: any) {
-  const doc = YAML.parseDocument(values);
+function parsePathAndValue(doc: YAML.ast.Document, path: string, value?: any) {
+  if (isEmpty(doc.contents)) {
+    // If the doc is empty we have an special case
+    return { value: set({}, path, value), splittedPath: [] };
+  }
   let splittedPath = path.split(".");
   // If the path is not defined (the parent nodes are undefined)
   // We need to change the path and the value to set to avoid accessing
@@ -88,13 +90,29 @@ export function setValue(values: string, path: string, newValue: any) {
   if (parentNode === undefined) {
     const definedPath = getDefinedPath(allElementsButTheLast, doc);
     const remainingPath = splittedPath.slice(definedPath.length + 1);
-    newValue = set({}, remainingPath.join("."), newValue);
+    value = set({}, remainingPath.join("."), value);
     splittedPath = splittedPath.slice(0, definedPath.length + 1);
   }
-  (doc as any).setIn(splittedPath, newValue);
+  return { splittedPath, value };
+}
+
+// setValue modifies the current values (text) based on a path
+export function setValue(values: string, path: string, newValue: any) {
+  const doc = YAML.parseDocument(values);
+  const { splittedPath, value } = parsePathAndValue(doc, path, newValue);
+  (doc as any).setIn(splittedPath, value);
   return doc.toString();
 }
 
+export function deleteValue(values: string, path: string) {
+  const doc = YAML.parseDocument(values);
+  const { splittedPath } = parsePathAndValue(doc, path);
+  (doc as any).deleteIn(splittedPath);
+  // If the document is empty after the deletion instead of returning {}
+  // we return an empty line "\n"
+  return doc.contents && !isEmpty((doc.contents as any).items) ? doc.toString() : "\n";
+}
+
 // getValue returns the current value of an object based on YAML text and its path
 export function getValue(values: string, path: string, defaultValue?: any) {
   const doc = YAML.parseDocument(values);