Issue 551: allow helm imagePullSecrets on post-install hook

[TomBillietKlarrio]

Fix for #551

[TomBillietKlarrio]

CI failed, but I doubt it's related to my change. Anybody an idea?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9fc6151) 85.91% compared to head (fe02e09) 85.91%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #554   +/-   ##
=======================================
  Coverage   85.91%   85.91%           
=======================================
  Files          12       12           
  Lines        1633     1633           
=======================================
  Hits         1403     1403           
  Misses        145      145           
  Partials       85       85           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[anishakj]

@TomBillietKlarrio Could you please signoff your commit?

[anishakj]

@TomBillietKlarrio Does it require similar change in https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper/templates/zookeeper.yaml#L107

[TomBillietKlarrio]

@TomBillietKlarrio Does it require similar change in https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper/templates/zookeeper.yaml#L107

I'm not quite sure what you mean here. This line refers to the serviceAccount of the zookeeper itself, which gets created by the zookeeper-operator itself I believe.
The parts I was touching were only related to the helm-post-intall-upgrade hook parts.

[anishakj]

@TomBillietKlarrio Does it require similar change in https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper/templates/zookeeper.yaml#L107

I'm not quite sure what you mean here. This line refers to the serviceAccount of the zookeeper itself, which gets created by the zookeeper-operator itself I believe. The parts I was touching were only related to the helm-post-intall-upgrade hook parts.

The line no: was not correct, in imagePullsecrets of service account, do we need to check for global value?

@TomBillietKlarrio Does it require similar change in https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper/templates/zookeeper.yaml#L107

I'm not quite sure what you mean here. This line refers to the serviceAccount of the zookeeper itself, which gets created by the zookeeper-operator itself I believe. The parts I was touching were only related to the helm-post-intall-upgrade hook parts.

I meant line no:108, there we are setting imagepullsecret , do we need to consider global imagepullsecret there

[TomBillietKlarrio]

@TomBillietKlarrio Does it require similar change in https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper/templates/zookeeper.yaml#L107

I'm not quite sure what you mean here. This line refers to the serviceAccount of the zookeeper itself, which gets created by the zookeeper-operator itself I believe. The parts I was touching were only related to the helm-post-intall-upgrade hook parts.

The line no: was not correct, in imagePullsecrets of service account, do we need to check for global value?

@TomBillietKlarrio Does it require similar change in https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper/templates/zookeeper.yaml#L107

I'm not quite sure what you mean here. This line refers to the serviceAccount of the zookeeper itself, which gets created by the zookeeper-operator itself I believe. The parts I was touching were only related to the helm-post-intall-upgrade hook parts.

I meant line no:108, there we are setting imagepullsecret , do we need to consider global imagepullsecret there

I see what you mean, I'll make a patch

[TomBillietKlarrio]

@anishakj I believe the PR is ready

[anishakj]

Below code block is not required looks like, service account is mentioned at line no:56

[TomBillietKlarrio]

It is. It's used here:
https://github.com/klarrio-kpn-dsh/zookeeper-operator/blob/fe02e094a4c2f640dbecd56d4424cc1b58c86d25/charts/zookeeper/templates/post-install-upgrade-hooks.yaml#L41

And in line with the zookeeper-operator values:
https://github.com/klarrio-kpn-dsh/zookeeper-operator/blob/fe02e094a4c2f640dbecd56d4424cc1b58c86d25/charts/zookeeper-operator/values.yaml#L27

[anishakj]

referring zookeeper/values.yaml not the zookeeper-operator

[jkhalack]

Line 56 here has pod.imagePullSecrets (used on ZookeeperCluster resource).
The proposed addition would specify serviceAccount.imagePullSecrets (to be used on the hooks).
@TomBillietKlarrio Why don't we move this to hooks.serviceAccount.imagePullSecrets to make it clear that the particular set of pull secrets would be pertinent to the hook image docker registry?

[anishakj]

@jkhalack Could you please have a look at this PR

[jkhalack]

This would constitute a breaking change. Do we have grounds for it?
The schema for .Values.pod.imagePullSecrets seems to require an array of key-value pairs, while proposed schema for .Values.global.imagePullSecrets seems to an array of strings.