fixed output to be triggered also by different addon.Version bsc#1167320

[atighineanu]

Why is this PR needed?

'skuba addon upgrade plan' doesn't ouput anything if there is a diff just in addon.Version;
it seems to be triggered only by addon.ManifestVersion;

• Description:

BEFORE:

--> When I change in configmap the value of addon.Version:

*configmaps: skuba-config

apiVersion: v1
data:
  SkubaConfiguration: |
    AddonsVersion:
      cilium:
        ManifestVersion: 2
        Version: 1.5.3
      dex:
        ManifestVersion: 5
        Version: 1.9.0      <------HERE: I can put literally any value and there won't be any upgrade output (original value was 2.16.0)
      gangway:
        ManifestVersion: 4
        Version: 3.1.0-rev4
      kured:
        ManifestVersion: 2
        Version: 1.2.0-rev4
      psp:
        ManifestVersion: 1
        Version: ""
kind: ConfigMap
metadata:
  creationTimestamp: "2020-03-20T16:15:47Z"
  name: skuba-config
  namespace: kube-system
  resourceVersion: "149078"
  selfLink: /api/v1/namespaces/kube-system/configmaps/skuba-config
  uid: 99f57658-0edc-44cd-a969-d2b1c0fcfaf7

--> then I run skuba addon upgrade plan

73b305ee50a5:~/go/src/skuba-dev/imba-cluster # skuba addon upgrade plan
** This is an UNTAGGED version and NOT intended for production usage. **
Current Kubernetes cluster version: 1.15.2
Latest Kubernetes version: 1.17.4

Congratulations! Addons for 1.15.2 are already at the latest version available

--> As you can see, no plan for DEX is outputed. However, if I change the addon.ManifestVersion, it will trigger output, BUT ONLY for addon.Version.

--> Now, I will change also the manifest for dex in skuba-config configmap:

*configmaps: skuba-config

apiVersion: v1
data:
  SkubaConfiguration: |
    AddonsVersion:
      cilium:
        ManifestVersion: 2
        Version: 1.5.3
      dex:
        ManifestVersion: 4  <------HERE! Now I changed the Manifest Version (original value: 5);
        Version: 1.9.0
      gangway:
        ManifestVersion: 4
        Version: 3.1.0-rev4
      kured:
        ManifestVersion: 2
        Version: 1.2.0-rev4
      psp:
        ManifestVersion: 1
        Version: ""
kind: ConfigMap
metadata:
  creationTimestamp: "2020-03-20T16:15:47Z"
  name: skuba-config
  namespace: kube-system
  resourceVersion: "149078"
  selfLink: /api/v1/namespaces/kube-system/configmaps/skuba-config
  uid: 99f57658-0edc-44cd-a969-d2b1c0fcfaf7

--> Then I run skuba addon upgrade plan

73b305ee50a5:~/go/src/skuba-dev/imba-cluster # skuba addon upgrade plan
** This is an UNTAGGED version and NOT intended for production usage. **
Current Kubernetes cluster version: 1.15.2
Latest Kubernetes version: 1.17.4

Addon upgrades for 1.15.2:
  - dex: 1.9.0 -> 2.16.0

--> Now, I will run again the skuba addon upgrade plan, after I correct the dex.Version in the skuba-config configmap (dex.Version = 2.16.0):

73b305ee50a5:~/go/src/skuba-dev/imba-cluster # skuba addon upgrade plan
** This is an UNTAGGED version and NOT intended for production usage. **
Current Kubernetes cluster version: 1.15.2
Latest Kubernetes version: 1.17.4

Addon upgrades for 1.15.2:
  - dex: 2.16.0 (manifest version from 4 to 5)

--> As you can see, it prints whether only the version or the manifest

=======================

AFTER:

--> for these values of dex.ManifestVersion and .Version in skuba-config configmap:

      dex:
        ManifestVersion: 4       <--(must be 5)
        Version: 2.14.0              <--(must be 2.16.0)

--> it will report both, the manifest version AND the version:

73b305ee50a5:~/go/src/skuba-dev/imba-cluster # skuba addon upgrade plan
** This is an UNTAGGED version and NOT intended for production usage. **
Current Kubernetes cluster version: 1.15.2
Latest Kubernetes version: 1.17.4

Addon upgrades for 1.15.2:
  - dex: 2.14.0 -> 2.16.0
       (manifest version from 4 to 5)

-> NOW, we fix the manifest back to see if 'skuba upgrade plan' shows anything only if addon.Version is outdated:

(in skuba-config configmap)

      dex:
        ManifestVersion: 5
        Version: 2.14.0

-> we run skuba addon upgrade plan

73b305ee50a5:~/go/src/skuba-dev/imba-cluster # skuba addon upgrade plan
** This is an UNTAGGED version and NOT intended for production usage. **
Current Kubernetes cluster version: 1.15.2
Latest Kubernetes version: 1.17.4

Addon upgrades for 1.15.2:
  - dex: 2.14.0 -> 2.16.0

Fixes #

https://bugzilla.suse.com/show_bug.cgi?id=1167320

What does this PR do?

it fixes the output of 'skuba addon upgrade plan' in case there is only an addon version to upgrade. Previously it was triggered only by the addon manifestversion.

Anything else a reviewer needs to know?

Testcase Implemantation here:
https://github.com/atighineanu/bdd-poc/blob/master/features/skuba_upgrade/skuba_upgrade-bsc%231167320.feature

Info for QA

How to reproduce:
BEFORE:

kubectl edit configmaps skuba-config -n kube-system

(change **Just** the Version of an Addon, any addon you pick)

expect:

skuba addon upgrade plan

(Skuba will inform you everything is up-to-date)

Now, change also the ManifestVersion of the same addon you modified the Version.

kubectl edit configmaps skuba-config -n kube-system

expect:

skuba addon upgrade plan

(Skuba will show you only the Version upgrade)

AFTER:

kubectl edit configmaps skuba-config -n kube-system

(change **Just** the Version of an Addon, any addon you pick)

expect:

skuba addon upgrade plan

(Skuba will show you there is an addon to be upgraded)

Now, change also the ManifestVersion of the same addon you modified the Version.

kubectl edit configmaps skuba-config -n kube-system

expect:

skuba addon upgrade plan

(Skuba will show you both, the ManifestVersion upgrade and Version upgrade for the addon)

Related info

Info that can be relevant for QA:

• link to other PRs that should be merged together
• link to packages that should be released together
• upstream issues

Status BEFORE applying the patch

How can we reproduce the issue? How can we see this issue? Please provide the steps and the prove
this issue is not fixed.

Status AFTER applying the patch

How can we validate this issue is fixed? Please provide the steps and the prove this issue is fixed.

Docs

If docs need to be updated, please add a link to a PR to https://github.com/SUSE/doc-caasp.
At the time of creating the issue, this PR can be work in progress (set its title to [WIP]),
but the documentation needs to be finalized before the PR can be merged.

Merge restrictions

(Please do not edit this)

We are in v4-maintenance phase, so we will restrict what can be merged to prevent unexpected surprises:

What can be merged (merge criteria):
    2 approvals:
        1 developer: code is fine
        1 QA: QA is fine
    there is a PR for updating documentation (or a statement that this is not needed)

[ereslibre]

Thank you @atighineanu for the patch :)

This behavior happens by design, the manifest version should always be bumped (even if nothing changes on the manifest itself -- e.g. you are only bumping the version of the addon).

For me, being forced to always bump the manifest version is easier in terms of rules: you know that everytime you touch anything on an addon, you always have to bump the number. Having more implicit rules is more complex in my opinion, for example: if you change the manifest you bump the manifest version, if you only change the version, there is no need to change the manifest version.

I see value though, in that I believe some contributors expect that just bumping the version would be enough, and that they don't need to artificially bump the manifest version every time.

Since it's not a very pressing issue I'd like to dig a little deeper, because I'm mainly concerned about some logic that we have regarding the addon comparison, and something could be missing here.

All in all, I'm fine in introducing this, so there's no need to always bump the manifest version, but I believe this requires a thorough review.

[jenting]

personally I like this idea, I might forget to bump manifest version when only container image version tag change.

[evrardjp]

personally I like this idea, I might forget to bump manifest version when only container image version tag change.

Maybe we can introduce a git hook to check this. Or a CI check. Or both.

[chentex]

By design the add-on and the manifest version should be bumped together, I think this can be done with a check to avoid mistakes, like @evrardjp said.