Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added imagePullSecrets to serviceAccount for images in a private registry #159

Merged
merged 2 commits into from
Jun 16, 2020

Conversation

azun
Copy link
Contributor

@azun azun commented Apr 2, 2020

Adding the option imagePullSecrets inside the serviceAccount to allow for images in private registries. A list of secret names can be specified in serviceAccount.imagePullSecrets

@azun azun force-pushed the operator-image-secrets branch from e8378e5 to 600f01c Compare April 2, 2020 10:51
@spiegela spiegela self-requested a review April 17, 2020 20:37
@spiegela
Copy link
Contributor

@azun Thanks for this. I have also seen this field in other charts labeled registrySecret. Could we rename to this for consistency with ad-hoc Helm standards?

Also, this operator is frequently included as a subchart to others, since ZK is often a component of a larger application. For your use-cases, would it work to nest this value beneath a global block like:

global:
  registrySecret:  <secret-name>

That way this value could be set once in the parent chart?

@azun
Copy link
Contributor Author

azun commented Jun 9, 2020

Nesting the secrets under global sounds like a good idea.

I've looked at charts in the official helm/charts github repo and most use a combination of .Values.global.imagePullSecrets and/or .Values.imagePullSecrets/.Values.serviceAccount.imagePullSecrets. I've added global to the chart template, with the option to overwrite it from inside the serviceAccount parameter.

I haven't seen registrySecret used before and imagePullSecrets is more clear in my opinion.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #159 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #159   +/-   ##
=======================================
  Coverage   82.41%   82.41%           
=======================================
  Files          11       11           
  Lines        1194     1194           
=======================================
  Hits          984      984           
  Misses        142      142           
  Partials       68       68           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ded6a82...5e706d5. Read the comment docs.

@anishakj anishakj requested a review from SrishT June 10, 2020 03:58
Copy link
Contributor

@SrishT SrishT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@anishakj
Copy link
Contributor

Could you please rebase the changes with master, after that we can merge

@azun azun force-pushed the operator-image-secrets branch from c14a2b9 to 5e706d5 Compare June 16, 2020 11:08
@azun
Copy link
Contributor Author

azun commented Jun 16, 2020

rebased

@anishakj anishakj merged commit 509a2c3 into pravega:master Jun 16, 2020
@amuraru amuraru deleted the operator-image-secrets branch October 18, 2020 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants