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

Use global.imagePullSecrets at solr-operator helm chart #338 #343

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

j-coll
Copy link

@j-coll j-coll commented Oct 13, 2021

Same PR as #339, but against main branch

Copy link
Contributor

@HoustonPutman HoustonPutman left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this would indeed be very helpful for people!

Before merging would you mind adding a Changelog entry here:
https://github.com/apache/solr-operator/blob/main/helm/solr-operator/Chart.yaml#L56

And adding an entry to the helm chart documentation as well?
https://github.com/apache/solr-operator/blob/main/helm/solr-operator/README.md#running-the-solr-operator

@@ -39,6 +39,10 @@ spec:
spec:
securityContext:
runAsNonRoot: true
{{- if .Values.global.imagePullSecrets }}
imagePullSecrets:
{{- toYaml .Values.global.imagePullSecrets | nindent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove the first - here? We mainly aren't using that with toYamls in this file.

@j-coll
Copy link
Author

j-coll commented Oct 22, 2021

I saw a discrepancy between this "imagePullSecrets" and the one at the zookeeper-operator. I was expecting here a list of objects with "name", but the zookeeper-operator takes a list of strings

https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper-operator/values.yaml#L5-L8

Also, the operator is adding those to the service account, instead of the deployment itself:
https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper-operator/templates/service_account.yaml#L4-L9
https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper-operator/values.yaml#L23-L24

Which is more in line with your proposal here:
#339 (comment)

@hossman , do you have any preference on where to add the imagePullSecrets?
Should we try to be "as close as possible" to the zookeeper-operator?

@HoustonPutman HoustonPutman linked an issue Oct 28, 2021 that may be closed by this pull request
@HoustonPutman
Copy link
Contributor

Well if you want to share imagePullSecrets here, you can merely use the same serviceAccount that is created by the Zookeeper Operator with the Solr Operator. That way the imagePullSecrets will be available to both through the same serviceAccount.

serviceAccount:
  create: false
  name: operator-account

zookeeper-operator:
  install: true
  serviceAccount:
     create: true
     name: operator-account
     imagePullSecrets:
        - list
        - of
        - strings

Using the above helm values, the zookeeper-operator helm chart will create the serviceAccount with the listed imagePullSecrets. Then the Solr Operator will merely use the same one, named operator-account.

No need to actually use the global imagePullSecrets.

@piotrrybicki
Copy link

Hello there. @j-coll are you willing to do proper changes in this PR?
I'd love to have ability to provide imagePullSecrets in globals. I'm not using ZK operator.

I can also create my own PR about this issue.

@HoustonPutman
Copy link
Contributor

@piotrrybicki Please open your own PR. We'd love to support as many globals as possible, to make setup easy for everyone!

As for my comment above it would be great if the helm code could detect a list of strings and convert it to a list of objects where necessary.

@j-coll
Copy link
Author

j-coll commented Mar 30, 2023

Hi, sorry for the silence. Yes, I'll do the required changes

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.

Use global.imagePullSecrets at solr-operator helm chart
3 participants