Show diff when upgrading

[t-eckert]

Changes proposed in this PR:

• Show the user the difference between the current installation and the upgraded installation on upgrade.
• Some refactoring of common behaviors.
• Added a LoadChart function for loading Helm charts.

Skip to 1 minute in to see the diff.

Screen.Recording.2022-01-06.at.2.36.00.PM.mov

How I've tested this PR:

• Added tests for diffing and loading Helm charts.

How I expect reviewers to test this PR:

The best way to quickly see the change is to use the CLI to install the demo preset, then upgrade to the secure preset.

From ./cli,

go run . install -preset demo

Wait for install to complete. Then

go run . upgrade -preset secure

You will see the difference between the two displayed as a diff:

  connectInject:
    enabled: true
-   metrics:
-     defaultEnableMerging: true
-     defaultEnabled: true
-     enableGatewayMetrics: true
  controller:
    enabled: true
  global:
+   acls:
+     manageSystemACLs: true
+   gossipEncryption:
+     autoGenerate: true
-   metrics:
-     enableAgentMetrics: true
-     enabled: true
    name: consul
+   tls:
+     enableAutoEncrypt: true
+     enabled: true
- prometheus:
-   enabled: true
  server:
    replicas: 1
- ui:
-   enabled: true
-   service:
-     enabled: true

Checklist:

• Tests added
• CHANGELOG entry added

  HashiCorp engineers only, community PRs should not add a changelog entry.
  Entries should use present tense (e.g. Add support for...)

[ndhanushkodi]

This is looking great Thomas, thanks for writing the diffing logic to be so clear and readable!

Is it worth noting somewhere (docs, comments, etc) that the install overrides and the the diff output will always only show the difference between user-set overrides in the installation and user-set overrides in the upgrade, rather than only the values that are different from the defaults? (For example in the screenshots below, I explicitly set global.enabled=true during install and didn't set it while upgrading, but the default value of global.enabled is true anyways, but it still shows up in the diff).

One slight nit (also shown in the screenshots below) is that the values output for install is indented one tab over from the values output for upgrade. Also, install calls those values "Overrides". I think it makes sense to also use the title "Overrides" for the upgrade diff as well.

[ishustava]

Amazing work on this PR Thomas!! I really love the refactor and all the work on tests! I'm continuing to review today (and apologies for taking long) but wanted to leave comments I have so far.

I noticed a few things when running the command with -dry-run (full output below):

1. It displayed the summary twice, and only one of them had the diffing. I think we probably only need to display one with diffing.
2. It said Performing dry run upgrade twice. Once in the beginning without the ===> and then once after the first summary.
3. It was kind of stuck for a bit at the --> preparing upgrade for consul line, which seemed strange because the info it displayed wasn't that different from the one displayed by ==> Consul Upgrade Summary. I'm not sure if this is something that is expected.

Here is the full output:

$ go run . upgrade -preset demo -set server.replicas=3 -dry-run
    Performing dry run upgrade.

==> Pre-Upgrade Checks
 ✓ Existing installation found to be upgraded.
    Name: consul
    Namespace: consul
 ✓ Loaded charts

==> Consul Upgrade Summary
    Installation name: consul
    Namespace: consul
  connectInject:
    enabled: true
    metrics:
      defaultEnableMerging: true
      defaultEnabled: true
      enableGatewayMetrics: true
  controller:
    enabled: true
  global:
    metrics:
      enableAgentMetrics: true
      enabled: true
    name: consul
  prometheus:
    enabled: true
  server:
-   replicas: 1
+   replicas: 3
  ui:
    enabled: true
    service:
      enabled: true


==> Performing Dry Run Upgrade
 --> preparing upgrade for consul
 --> performing update for consul
 --> dry run for consul
    Dry run complete - upgrade can proceed.
    Config:
    connectInject:
      enabled: true
      metrics:
        defaultEnableMerging: true
        defaultEnabled: true
        enableGatewayMetrics: true
    controller:
      enabled: true
    global:
      metrics:
        enableAgentMetrics: true
        enabled: true
      name: consul
    prometheus:
      enabled: true
    server:
      replicas: 3
    ui:
      enabled: true
      service:
        enabled: true

[ishustava]

Finishing my review. A few more comments.

[t-eckert]

Is it worth noting somewhere (docs, comments, etc) that the install overrides and the the diff output will always only show the difference between user-set overrides in the installation and user-set overrides in the upgrade, rather than only the values that are different from the defaults? (For example in the screenshots below, I explicitly set global.enabled=true during install and didn't set it while upgrading, but the default value of global.enabled is true anyways, but it still shows up in the diff).

This is such an interesting question. I think the solution for now is what you said where there is a note "Diffing user overrides..." something like that. I think we should discuss the right approach for future iterations as a team. Should we show the whole diff with the whole helm chart or just the overrides? I could go either way.

[ishustava]

I think this looks good! Thanks for your patience as we were working through the UX and having long review cycles.

Leaving a few minor comments, but won't block.

[ndhanushkodi]

Looks good to me Thomas, I didn't try manually testing this time around, but let me know if you'd like me to!