Improve the UpgradeForm to migrate values from one version to other

[andresmgot]

Follow up from #1182

As discussed in #1144, it will be useful to improve the current behavior when upgrading charts. By default, when a user selects a new version, the current values get overwritten with the default values of the new version. From the point in which the user modifies any value, changing version takes no effect.

We should investigate a better way of migrating changes between versions and if possible show the difference that will be applied before submitting the upgrade.

[andresmgot]

[PROPOSAL]

This is my proposal of the possible user experience of a user upgrading the app MyApp from version 1.0.0 to 1.0.1:

1. When clicking in the "Upgrade" button (and after selecting the source repo if necessary), the user will be presented with the current status of the release: MyApp v1.0.0 with the values used when the app was first deployed.
2. The user clicks on the new version: v1.0.1.
   (option a.) The values are kept the same so changes made in the original release can be reused.
   (option b.) Instead of just leaving the original values, we parse the original values as a yaml Document. We can iterate over the elements of the doc and if the new version contains the same section, overwrite it with the previous values. If not, add it to the end.
3. We can render a new Tab representing the diff between the current values and the default values of the new version (ignore the text in the tabs):

4. When clicking "submit", we can show in a modal the diff between the default values of the version to upgrade and the values that will be submitted. The user can do a final review at that moment if that's the desired result.
5. Submit when confirmed.

Regarding option a. vs. option b. in the step 2, I would go for the first option because:

• It's less prone to errors. With automated changes, we are likely going to add/modify something that we shouldn't.
• Upgrades will require human intervention anyway. We don't know if we would be helping with the automated merge.
• It's easier to implement. If after going with option a. we still want to implement option b. we can do so.

Let me know what you think. cc/ @absoludity

BTW, for the Diff view, there is a useful react package: https://www.npmjs.com/package/react-diff-viewer

[absoludity]

[PROPOSAL]

This is my proposal of the possible user experience of a user upgrading the app MyApp from version 1.0.0 to 1.0.1:

1. When clicking in the "Upgrade" button (and after selecting the source repo if necessary), the user will be presented with the current status of the release: MyApp v1.0.0 with the `values` used when the app was first deployed.

2. The user clicks on the new version: v1.0.1.

I think I've asked this before, but why does the user need to explicitly click on the new version? If I click on the "Upgrade" button, I expect the UI to already have the latest version selected by this point - though I may want to modify it if there are other versions available.

   (option a.) The values are kept the same so changes made in the original release can be reused.

No matter what we do here, we always want to ensure that "changes made in the original release can be reused" don't we? That's the point of this work (so I'm not sure why it's used to justify option a.)

   (option b.) Instead of just leaving the original values, we parse the original values as a yaml `Document`. We can iterate over the elements of the doc and if the new version contains the same section, overwrite it with the previous values. If not, add it to the end.

I think Option b. is just saying, "merge the original values into the new chart version values", I think? (the implementation of how they are merged isn't so relevant here).

At this point, it looks like the information we really want is the diff (json patch) which was applied to the original install (if any). ie. what customizations did the user make when deploying. I assume we can get this as a diff easily enough though? (We have the original values.yaml, as well as what was deployed). If so, could we do:

(option b.) We merge the user customisations from the deployed values into the target chart version values?

3. We can render a new Tab representing the `diff` between the current values and the default values of the new version (ignore the text in the tabs):

I'm not sure if you mean the same thing (doesn't seem so), but thinking as a user I think the important information to know is what my own customisations are. So what I'd like to see here, as a user, is exactly that - a diff that highlights my customisations to the selected chart, both those carried over from my earlier deploy and any other changes I make here (and updated if I change the selected version).

It's not clear to me if that's exactly what you mean, or something slightly different.

Note that this diff would be useful generally, not just when upgrading, I think? (ie. even when I am configuring a new chart installation, it'd be useful to switch to a tab where I can see just the diff of exactly what I've customised.)

Nice.

1. When clicking "submit", we can show in a modal the diff between the default values of the version to upgrade and the values that will be submitted. The user can do a final review at that moment if that's the desired result.

Why do we need a second diff here, shown when the user submits. I want to be aware before I click submit of exactly what my customisations are.

2. Submit when confirmed.

Regarding option a. vs. option b. in the step 2, I would go for the first option because:

* It's less prone to errors. With automated changes, we are likely going to add/modify something that we shouldn't.

If we don't merge the previous customisations, at what point above does the user manually add them? It sounds like you want to do it after they click submit - in the modal? Or am I missing something?

* Upgrades will require human intervention anyway. We don't know if we would be helping with the automated merge.

There will be cases where we might apply customizations to the wrong section (ie. if the charm upgrade re-arranges the schema), but that should not be often, and importantly, it should be very clear in the result (which we highlight).

* It's easier to implement. If after going with option a. we still want to implement option b. we can do so.

I'm wary of this one :P.

Let me know what you think. cc/ @absoludity

I'm keen to find out whether we can easily identify user customisations, so we can know exactly what was customised with the original deploy, and are able to apply that to the targeted deploy.

BTW, for the Diff view, there is a useful react package: https://www.npmjs.com/package/react-diff-viewer

Excellent!

[andresmgot]

[PROPOSAL]
This is my proposal of the possible user experience of a user upgrading the app MyApp from version 1.0.0 to 1.0.1:

1. When clicking in the "Upgrade" button (and after selecting the source repo if necessary), the user will be presented with the current status of the release: MyApp v1.0.0 with the `values` used when the app was first deployed.

2. The user clicks on the new version: v1.0.1.

I think I've asked this before, but why does the user need to explicitly click on the new version? If I click on the "Upgrade" button, I expect the UI to already have the latest version selected by this point - though I may want to modify it if there are other versions available.

Right, I forgot about that. It's true that there is no reason for not choosing the latest.

   (option a.) The values are kept the same so changes made in the original release can be reused.

No matter what we do here, we always want to ensure that "changes made in the original release can be reused" don't we? That's the point of this work (so I'm not sure why it's used to justify option a.)

Correct, I am just specifying it to compare it to option b) that tries to merge values.

   (option b.) Instead of just leaving the original values, we parse the original values as a yaml `Document`. We can iterate over the elements of the doc and if the new version contains the same section, overwrite it with the previous values. If not, add it to the end.

I think Option b. is just saying, "merge the original values into the new chart version values", I think? (the implementation of how they are merged isn't so relevant here).

Yes.

At this point, it looks like the information we really want is the diff (json patch) which was applied to the original install (if any). ie. what customizations did the user make when deploying. I assume we can get this as a diff easily enough though? (We have the original values.yaml, as well as what was deployed). If so, could we do:

(option b.) We merge the user customisations from the deployed values into the target chart version values?

We could do that too but I see some problems with that (I have elaborated my concerns below)

3. We can render a new Tab representing the `diff` between the current values and the default values of the new version (ignore the text in the tabs):

I'm not sure if you mean the same thing (doesn't seem so), but thinking as a user I think the important information to know is what my own customisations are. So what I'd like to see here, as a user, is exactly that - a diff that highlights my customisations to the selected chart, both those carried over from my earlier deploy and any other changes I make here (and updated if I change the selected version).

It's not clear to me if that's exactly what you mean, or something slightly different.

What I mean is that we would show the difference between whatever was installed and the default values for the current chart version. This means that (assuming we don't implement option b.) if I installed 1.0.0 and I left everything as the default, when selecting 1.0.1 I will see the different between the default values of 1.0.0 (that will be installed if I do nothing) and the version 1.0.1.

The problem of working with "customisations" is that we cannot ensure that the patch can be applied to the new values.yaml (see more details below).

Note that this diff would be useful generally, not just when upgrading, I think? (ie. even when I am configuring a new chart installation, it'd be useful to switch to a tab where I can see just the diff of exactly what I've customised.)

Good point, yes. Maybe we can add this view to both deployments forms.

1. When clicking "submit", we can show in a modal the diff between the default values of the version to upgrade and the values that will be submitted. The user can do a final review at that moment if that's the desired result.

Why do we need a second diff here, shown when the user submits. I want to be aware before I click submit of exactly what my customisations are.

Yes, note that this second diff is equal to the first. This is just for confirmation. I thought of adding this since it's not mandatory to see the tab "Diff" so people could click submit without seeing the changes.

2. Submit when confirmed.

Regarding option a. vs. option b. in the step 2, I would go for the first option because:

* It's less prone to errors. With automated changes, we are likely going to add/modify something that we shouldn't.

If we don't merge the previous customisations, at what point above does the user manually add them? It sounds like you want to do it after they click submit - in the modal? Or am I missing something?

Not exactly. What I tried to explain is that the whole values.yaml (not only the customisations) of the existing versions would be used for the new version if the form is left untouched. If the values of the new version contain changes that needs to be added, that's the part that the user needs to copy manually to the new version. That can be tedious but at least you are sure you are not introducing unexpected values to the chart.

* Upgrades will require human intervention anyway. We don't know if we would be helping with the automated merge.

There will be cases where we might apply customizations to the wrong section (ie. if the charm upgrade re-arranges the schema), but that should not be often, and importantly, it should be very clear in the result (which we highlight).

Yes, that's why I didn't propose to work with customizations.

* It's easier to implement. If after going with option a. we still want to implement option b. we can do so.

I'm wary of this one :P.

Let me know what you think. cc/ @absoludity

I'm keen to find out whether we can easily identify user customisations, so we can know exactly what was customised with the original deploy, and are able to apply that to the targeted deploy.

I am not sure about working with customisations for two reasons:

• If the patch fails (for example if the values changes a lot) what would we do? I don't know if we could show a conflict like view like git.
• We will be hiding changes in the previous values if we just apply the patch. Let me put an example. Imagine the YAML:

foo: bar
bar: foo
...20 lines later
foobar: barfoo

Then the user changes it and adds a line:

  foo: bar
+ myvar: value
  bar: foo

If the next version changes the default foobar: null the diff view will only show the line added but not the change of the foobar variable. We would be showing just some changes from the previous version, not all of them.

[absoludity]

Let's chat through these next time we're on a call.

[absoludity]

Thanks for the chat. So something I mentioned there, which has become clearer thinking some more, is that I was not thinking of "diff" and "patch" as the same thing, where as I think you might be. Specifically, I was thinking of the visual diff only as the presentation of the changes to the user, whereas we would model those changes internally more accurately as an actual JSON Patch patch :) So for example:

I am not sure about working with customisations for two reasons:

* If the patch fails (for example if the values changes a lot) what would we do? I don't know if we could show a `conflict` like view like git.

Using a JSON Patch, since it is just an array of operations, it should be simple to apply those that work and those that don't (ie. where structures have been modified/renamed) we can convert into a friendly comment to put at the top of the new values.

* We will be hiding changes in the previous values if we just apply the patch. Let me put an example. Imagine the YAML:

foo: bar
bar: foo
...20 lines later
foobar: barfoo

Then the user changes it and adds a line:

  foo: bar
+ myvar: value
  bar: foo

As a JSON Patch, this would be something like:

{ "op": "add", "path": "/myvar", "value": "value" },

so that...

If the next version changes the default foobar: null the diff view will only show the line added but not the change of the foobar variable. We would be showing just some changes from the previous version, not all of them.

it should apply cleanly (unlike trying to apply a textual diff). But as you mentioned, that may bring us back to the issue of comments (ie. the yaml library we're using supports them, but other libs like https://github.com/Starcounter-Jack/JSON-Patch, which looks great for generating the patches, may not).

That said, we could generate the JSON patch using something like the above, then applying that patch with the existing yaml library would be pretty trivial, I think? (given that the json patch specifies the path etc.) Worth checking, perhaps.

[andresmgot]

Okay, let me explore that possibility. I am not sure how that might behave with "corner" cases like adding or changing elements in an array.

[andresmgot]

That looks indeed promising! Using JSON patch, we avoid the issues I highlighted about applying changes as raw text. It just require some small changes but I am not 100% sure we won't be entering some bugs in the process. To help with that we can add a button "Restore Defaults" that sets the values back to the version default values (which is the current behavior).

So to summarize, this is what I plan to do:

1. Get the JSON patch between the original values (for the original version) and the deployed values.
2. When a new version is selected, apply the JSON patch to that version.
3. Add a new button "Restore Defaults" that sets the values.yaml to the default of that version.
4. Add the Diff tab showing the differences between the current deployed values and the values that are going to be deployed.
5. (Optional?) Show the diff before submitting for confirmation.

Let me know what you think, I am going to start sending PR about those ^

[absoludity]

That looks indeed promising! Using JSON patch, we avoid the issues I highlighted about applying changes as raw text.

Excellent.

It just require some small changes but I am not 100% sure we won't be entering some bugs in the process. To help with that we can add a button "Restore Defaults" that sets the values back to the version default values (which is the current behavior).

So to summarize, this is what I plan to do:

1. Get the JSON patch between the original values (for the original version) and the deployed values.

2. When a new version is selected, apply the JSON patch to that version.

3. Add a new button "Restore Defaults" that sets the `values.yaml` to the default of that version.

4. Add the Diff tab showing the differences between the current deployed values and the values that are going to be deployed.

This is the only point I was unsure about: I had thought that as a user, what I'm interested in is the differences between the new chart's defaults and the values I'm going to deploy (ie. what are my customisations)... but I think your point is that this could hide something like the chart author changing a default which I might want to know about.

So I think you're right, it's actually more important to me to know what's changing in my actual deployment - whether from my own customisations or from changes to chart default values etc. Any customisations that I had with the original deploy, which are applied fine, will not be displayed in this diff, but further customisations which I add will be. Yep, I think this is the right way to go. Thanks!

5. (Optional?) Show the diff before submitting for confirmation.

Yeah, still not sure this is necessary. There are a couple of things we could do at that point if we find it's not obvious enough. Let's look at it at that point.

Let me know what you think, I am going to start sending PR about those ^

Great - start sending :) Thanks Andres!

[absoludity]

2. When a new version is selected, apply the JSON patch to that version.

Just remember that a new version shouldn't need to be selected normally (ie. we can automatically select the latest available, while allowing the user to change this).

[andresmgot]

1. When a new version is selected, apply the JSON patch to that version.

Just remember that a new version shouldn't need to be selected normally (ie. we can automatically select the latest available, while allowing the user to change this).

Yes, I plan to do so in following PRs (first it's easier from an implementation point of view to start showing the old version in the first load).

[andresmgot]

Closing this since all the elements in the proposal have been implemented!