Revert repository and tag values.yaml fields to avoid breaking changes

diverdane commented 3 years ago

Is your feature request related to a problem? Please describe.

The changes that were made to the values.yaml file in order to support OpenShift deployments (PR #71) represent breaking changes for anyone using the Conjur OSS Helm chart. For example, the fields for the Conjur image repository/tag now use a kubernetes and openshift in the values path:

image:
  kubernetes:
    repository: cyberark/conjur  # https://hub.docker.com/r/cyberark/conjur/
    tag: '1.11.1'
  openshift:
    repository: registry.connect.redhat.com/cyberark/conjur
    tag: latest
  pullPolicy: Always

We would like to avoid breaking changes, and thereby limit the cadence of introducing major versions. In this case, we can avoid breaking changes by reverting the repository and tag fields to their original form, with default values of cyberark/conjur and 1.11.1 respectively. The default values to use for OpenShift would then need to be clearly documented both in the chart's README.md, and ideally in the values.yaml file itself.

Describe the solution you would like

The image.repository and image.tag value schema is restored.
Defaults for repository and tag are reverted to the Kubernetes defaults.
OpenShift default values are documented in the chart README.md and in the values.yaml file.

Describe alternatives you have considered

Separate default values fields for OpenShift, but this is complex and awkward looking.

Additional context

izgeri commented 3 years ago

@diverdane why do we need a whole separate field for the openshift image? to me, this looks weird:

image:
  repository: cyberark/conjur  # https://hub.docker.com/r/cyberark/conjur/
  tag: '1.11.1'
  pullPolicy: Always
. . .
openshift:
  image:
    repository: registry.connect.redhat.com/cyberark/conjur
    tag: latest
    pullPolicy: Always

in practice, I'm only going to deploy this to one platform, right?

what is the purpose of a separate field for openshift images if it uses the same sub-fields? is this a flag to trigger the OCP deployment instead of the vanilla k8s?

would it make more sense to have an optional platform sub-field of image, that can default to kubernetes and run the standard flow, but can be set to openshift if deploying to openshift? eg

image:
  repository: registry.connect.redhat.com/cyberark/conjur
  tag: latest
  pullPolicy: Always
  platform: openshift

diverdane commented 3 years ago

@izgeri: The intent is to incorporate two sets of default repository/tag values, one for Kubernetes and one for OpenShift. Current users of the chart will be depending on the default value of the Conjur repository to be cyberark/conjur, whereas any OpenShift users will want a default value of registry.connect.redhat.com/cyberark/conjur.

What we could potentially do is just introduce new OpenShift values within the image: section, e.g.:

image:
    repository: cyberark/conjur
    tag: '1.11.1'
    openshiftRepository: registry.connect.redhat.com/cyberark/conjur
    openshiftTag: latest
    pullPolicy: Always
    platform: openshift

or, probably cleaner:

image:
    repository: cyberark/conjur
    tag: '1.11.1'
    openshift:
        repository: registry.connect.redhat.com/cyberark/conjur
        tag: latest
    pullPolicy: Always
    platform: openshift

When we're ready to make a major version bump, we can probably change it back to the way the code is now (i.e. separate kubernetes: and openshift: sections).

izgeri commented 3 years ago

Can I challenge the idea that we need a default OpenShift image? Can't we just provide clear instructions that when you want to deploy to OCP, you need to override the repository field to point to registry.connect.redhat.com/cyberark/conjur (and also for the Nginx image)?

diverdane commented 3 years ago

Ah, yes, that makes sense, and would keep things very simple in the chart! This can be documented both in the README.md and in the values.yaml file itself (with commented values) to make it very clear.

Of course, the platform: field wouldn't offer much here. The user would still need to explicitly supply the image repo:tag values for OpenShift, despite the existence of a platform: openshift field.

cyberark / conjur-oss-helm-chart