fix(argo-workflows): container and mainContainer imagePullPolicy

[BaptisteDeporte]

Checklist:

• I have bumped the chart version according to versioning
• I have updated the documentation according to documentation
• I have updated the chart changelog with all the changes that come with this pull request according to changelog.
• Any new values are backwards compatible and/or have sensible default.
• I have signed off all my commits as required by DCO.
• My build is green (troubleshooting builds).

I found a strange behavior with the mainContainer "imagePullPolicy" (and the same with executor since they work similary).

With the current release, if I apply the chart with the following values.yaml

mainContainer:
  imagePullPolicy: "Always"

My controller config-map does not have the expecting output :

data:
  config: |
    nodeEvents:
      enabled: true
    workflowEvents:
      enabled: true

When we only set imagePullPolicy option for the mainContainer or the executor, it’s not applied.

Plus, the doc specifies that the default value for those fields are "Values.images.pullPolicy". So, I expect the config map to contains according pull policy for executor and main container when I apply the chart with an empty values.yaml, but it's not the case, I obtain the same config map mentioned earlier.

It’s my first contribution, I have read the CONTRIBUTING.md but any feedback is welcome !

[tico24]

Not sure this will work any more will it?

You need to add the if .Values.mainContainer.resources back in here for example. repeat for all the other things.

[BaptisteDeporte]

Well, I re-read my MR description, and it seems it was unclear my bad. I updated it so please, take a look

With this modification, I expect the config map to contains a "mainContainer" in any case with :
a. if they are no "mainContainer.imagePullPolicy" in the values files, I expect it to be the same as "Values.images.pullPolicy"
b. if they are a "mainContainer.imagePullPolicy" in the values file, I want the according values to match
(and same with executor)

The issue with the current release is this test :

{{- if or .Values.mainContainer.resources .Values.mainContainer.env .Values.mainContainer.envFrom .Values.mainContainer.securityContext}}

It don't work if we only set "mainContainer.imagePullPolicy" value. But if we trick the chart with a dummy resources for example, the test is triggered and we got a correct pull policy (the one specified by the user, and the images.pullPolicy as a fallback)

I run the followings tests, and it seems to work as described bellow :

With an empty values.yaml

With the current release (0.45.4) we obtain this config-map :

data:
  config: |
    nodeEvents:
      enabled: true
    workflowEvents:
      enabled: true

And with this chart, this one :

data:
  config: |
    mainContainer:
      imagePullPolicy: Always
    executor:
      imagePullPolicy: Always
    nodeEvents:
      enabled: true
    workflowEvents:
      enabled: true

With a pullPolicy set

With the following values.yaml :

mainContainer:
  imagePullPolicy: "IfNotPresent"

With the current release, we obtain the same config-map as before. And with this chart we obtain :

data:
  config: |
    mainContainer:
      imagePullPolicy: IfNotPresent
    executor:
      imagePullPolicy: Always
    nodeEvents:
      enabled: true
    workflowEvents:
      enabled: true

I made those tests with a freshly installed minikube.

[tico24]

This would also cause errors now if there's data missing. .Values.mainContainer is too broad

[BaptisteDeporte]

With this chart, Argo Workflow runs with an empty values.yaml (or with wrong keys) and is able to run workflows (tested with minikube, latest release and this dummy workflow).

But I understand that we want something more predictable, so I can rollback to the more specific test (with mainContainer.resources, mainContainer.securityContext etc...)