pravega / zookeeper-operator

Kubernetes Operator for Zookeeper
Apache License 2.0
368 stars 206 forks source link

Issue 551: allow helm imagePullSecrets on post-install hook #554

Open TomBillietKlarrio opened 1 year ago

TomBillietKlarrio commented 1 year ago

Fix for https://github.com/pravega/zookeeper-operator/issues/551

TomBillietKlarrio commented 1 year ago

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

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #554 +/- ## ======================================= Coverage 85.91% 85.91% ======================================= Files 12 12 Lines 1633 1633 ======================================= Hits 1403 1403 Misses 145 145 Partials 85 85 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

anishakj commented 11 months ago

@TomBillietKlarrio Could you please signoff your commit?

anishakj commented 11 months ago

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

TomBillietKlarrio commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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 commented 10 months ago

@anishakj I believe the PR is ready

anishakj commented 10 months ago

@jkhalack Could you please have a look at this PR