You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/03/02 21:09:52 UTC
[GitHub] [lucene-solr-operator] chaicesan opened a new pull request #231: Add conditional dependency for zk-operator helm chart
chaicesan opened a new pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231
This PR is the first of two phases that will allow us to make use of up-to-date versions of the zk-operator as well as being able to install it using a conditional helm chart dependency.
The aim of this PR is just to add the ability to install the latest version of the zk-operator via a helm chard dependency. Later work will involve making use of the new latest zk crd.
There were some pain points when it came to updating the dependencies, mainly due to compatibility involving `k8s.io/client-go`. I'm still pretty new to go and go modules so let me know if I am doing anything suspect.
This is my first PR to this project so please let me know if I have missed anything.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] HoustonPutman commented on pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#issuecomment-792876296
This is starting to look very good! I'll give it a test locally soon, to make sure that there are no issues with the Zookeeper Cluster that is created in the new version of the zookeeper-operator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] chaicesan commented on a change in pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
chaicesan commented on a change in pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#discussion_r589055356
##########
File path: helm/solr-operator/Chart.yaml
##########
@@ -95,4 +95,10 @@ annotations:
name: "example"
numThreads: 4
image:
- tag: 8.7.0
\ No newline at end of file
+ tag: 8.7.0
+
+dependencies:
+ - name: 'zookeeper-operator'
+ version: 0.2.9
+ repository: https://charts.pravega.io
+ condition: useZkOperator
Review comment:
Tell me what you think of the names I have chosen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] HoustonPutman commented on pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#issuecomment-797149720
I think I'm good now. Will let it sit for a day or two, and then merge! Reach out if you think there is anything missing.
Once again, thanks for all of your great work @chaicesan!!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] HoustonPutman commented on pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#issuecomment-797106816
I think we are very close to having this merge ready.
I updated the zk-connection string to use the new URLs used by this version of the ZK Operator. We just need to make all the other fields update cleanly. We should probably auto-populate the kubeDomain field from the SolrCloud Spec into the ZK Cluster spec as well... But that's very very easy.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] chaicesan commented on pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
chaicesan commented on pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#issuecomment-795679575
@HoustonPutman Sorry for the delay. I agree that it can be done on release so I will remove them today.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] HoustonPutman commented on pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#issuecomment-795697523
No worries at all @chaicesan , I've mainly just been taking care of random documentation and licensing issues.
I think this is almost ready to go. I'm going to do some testing first to make sure that we don't need to change the Solr operator code at all, and everything runs smoothly. I have a bet we will need to do at least one thing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] chaicesan commented on pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
chaicesan commented on pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#issuecomment-797423991
Nice one! Thanks for your help on this too @HoustonPutman :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] HoustonPutman commented on pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#issuecomment-794557931
Hey @chaicesan , I went ahead and changed the variable name myself, and added backwards compatibility with the old `useZkOperator` option. I also updated various docs across the project.
I'm not so sure about using Chart.lock and keeping the `charts/zookeeper-operator-0.2.9.tgz` and `Chart.lock` in the repo. That's something that can be generated at release time, when building the chart. Is there a reason why you included it in the PR specifically?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] HoustonPutman merged pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
HoustonPutman merged pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] HoustonPutman commented on a change in pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#discussion_r588764883
##########
File path: helm/solr-operator/Chart.yaml
##########
@@ -95,4 +95,10 @@ annotations:
name: "example"
numThreads: 4
image:
- tag: 8.7.0
\ No newline at end of file
+ tag: 8.7.0
+
+dependencies:
+ - name: 'zookeeper-operator'
+ version: 0.2.9
+ repository: https://charts.pravega.io
+ condition: useZkOperator
Review comment:
We probably want to change this to a different variable, such as `zookeeper-operator.enabled`, because we want users to be able to use the zookeeper-operator, without installing it as a dependency. That requires two variables. Then when setting the command line variable for solr, we can do an OR of `zookeeper-operator.enabled` and `useZkOperator`.
I'm flexible on names, if you don't think that's clear.
##########
File path: config/dependencies/zk_operator.yaml
##########
@@ -0,0 +1,37 @@
+apiVersion: apiextensions.k8s.io/v1beta1
Review comment:
This should be the v0.2.9 version:
https://github.com/pravega/zookeeper-operator/blob/v0.2.9/deploy/crds/zookeeper.pravega.io_zookeeperclusters_crd.yaml
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr-operator] HoustonPutman commented on a change in pull request #231: Add conditional dependency for zk-operator helm chart
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #231:
URL: https://github.com/apache/lucene-solr-operator/pull/231#discussion_r589558078
##########
File path: helm/solr-operator/Chart.yaml
##########
@@ -95,4 +95,10 @@ annotations:
name: "example"
numThreads: 4
image:
- tag: 8.7.0
\ No newline at end of file
+ tag: 8.7.0
+
+dependencies:
+ - name: 'zookeeper-operator'
+ version: 0.2.9
+ repository: https://charts.pravega.io
+ condition: useZkOperator
Review comment:
[The helm docs](https://helm.sh/docs/chart_best_practices/dependencies/#conditions-and-tags) give some good guidance. I think this logic is very good in general. We should just use the name `zookeeper-operator` instead of `zookeeperOperator`, since that's the name of the dependency.
We should also think about maybe including backwards compatibility of the old `useZookeeperOperator` option, which was unfortunately a string `"true"`.
I can take a stab at updating the documentation around this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org