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

solr-operator Helm chart #60

Closed
wants to merge 6 commits into from

Conversation

kcmartin
Copy link
Contributor

@kcmartin kcmartin commented Dec 10, 2019

Signed-off-by: Kristin Martin [email protected]

*Issue number of the reported bug or feature request: #20

Describe your changes
Adding Helm chart to install solr-operator.
For now, this lives within the solr-operator repo.

Testing performed
I tested installation on a local dev cluster, and on one of our dev clusters in AWS

Tested installation and removal using both Helm 2 and Helm 3, see notes README regarding CRD handling in Helm 2 vs. 3

Additional context
@swarupdonepudi mentions we will need to add RBAC to the chart, so I would like to get feedback from @HoustonPutman and @sepulworld regarding this.

Thank you!

@HoustonPutman
Copy link
Contributor

HoustonPutman commented Dec 12, 2019

Thanks for the addition kristin! I'm not very knowledgable about Helm at all, but I will try to learn as I review this. Sorry if this takes some time though. I'll probably have more than a few questions.

As for your last question, we will definitely need to add the RBAC rules to any deployment of the operator, otherwise the operator will fail trying to do any kind of operation.

@kcmartin
Copy link
Contributor Author

Thanks @HoustonPutman!

We also have @sepulworld and @swarupdonepudi on the team to assist with Helm-related questions.

I'll circle back with them about adding the RBAC rules.

One more note, It looks like the Travis build is failing due to kustomize being missing as a dependency, is that something I should add to https://github.com/bloomberg/solr-operator/blob/master/hack/install_dependencies.sh ?

@HoustonPutman
Copy link
Contributor

Yeah, for kustomize that's the right way to go.

I'd use the install method mentioned here: https://github.com/kubernetes-sigs/kustomize/blob/master/docs/INSTALL.md#try-go

Signed-off-by: Kristin Martin <[email protected]>


zkOperator: "true"
etcOperator: "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should default the etcOperator to false. My mind could be changed though. Until someone thouroughly checks that the functionality works (which I'm not sure anyone has ever used.....), I'd be more comfortable with people having to explicitly enable it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that the variables would better be named useZkOperator and useEtcdOperator, as that's what they really mean.

And it's etcd not etc, I see that a few times in this PR


zkOperator: "true"
etcOperator: "true"
ingressBaseDomain: ing.local.domain
Copy link
Contributor

Choose a reason for hiding this comment

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

This default should be empty. If this non-existent domain is the default, it will cause people starting the operator up to not be able to use their solrClouds, because the solrClouds will try registering as Urls that don't exist.

image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
imagePullPolicy: {{ .Values.image.pullPolicy }}
args:
- -zk-operator={{ if eq .Values.zkOperator "zooKeeper" }}true{{ else }}false{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by these, shouldn't .Values.zkOperator and .Values.etcOperator be booleans?

@kcmartin
Copy link
Contributor Author

I'm going to go ahead and close this PR, in favor of a new one that is cleaner and doesn't include weird merges :)

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.

2 participants