helm / charts

⚠️(OBSOLETE) Curated applications for Kubernetes
Apache License 2.0
15.49k stars 16.79k forks source link

[incubator/kafka] zookeeper dependency #9963

Closed fabriziofortino closed 5 years ago

fabriziofortino commented 5 years ago

Which chart: [incubator/kafka]

The current zookeeper dependency points to incubator/zookeeper. This chart is based on https://github.com/kubernetes/contrib/tree/master/statefulsets/zookeeper and the last image is on zookeeper 3.4.10 (released more than a year ago).

@cablespaghetti has opened a PR to update the image to the latest stable version that includes important fixes, like this one. Looking at this PR comment it seems that kubernetes/contrib is not maintained.

Since the kafka chart uses confluentinc/cp-kafka as image I was wondering if it would be better to create a separate chart (eg: [incubator/zookeeper-cp]) that uses confluentinc/cp-zookeeper image and have this as a dependency in the requirements.

@benjigoldberg what do you think?

bluebenno commented 5 years ago

I am working on a Kafka + Zookeeper project. I have a similar question. Perhaps an alternative is to modify the incubator/zookeeper chart to use the confluent docker image?

My specific feature requirement is jaas authentication and ACLs

benjigoldberg commented 5 years ago

@fabriziofortino @bluebenno I definitely agree that the ZK image should be bumped. Is there a reason why you would prefer the confluent zookeeper vs the standard zookeeper docker image?

fabriziofortino commented 5 years ago

@benjigoldberg I would personally prefer to have a standard zookeeper docker image (since the confluent one is released only when confluent releases a new version of the cp platform). Unfortunately, there is no official standard zk image. The one used in the chart is outdated and is part of a repo that does not seem to be supported anymore.

benjigoldberg commented 5 years ago

@fabriziofortino I think we could like adopt this one https://hub.docker.com/_/zookeeper/ though to be fair I have not investigated what changes, if any, would be necessary to run this in k8s.

cablespaghetti commented 5 years ago

Unfortunately my PR to update the image we're using does introduce so unexplained instability which is highly annoying.

It would be good to move to the "official" one from docker hub or maybe something based on it. I'll have a go at it when I find myself with some time if nobody else does sooner.

On Thu, 10 Jan 2019, 15:03 Benjamin Goldberg <notifications@github.com wrote:

@fabriziofortino https://github.com/fabriziofortino I think we could like adopt this one https://hub.docker.com/_/zookeeper/ though to be fair I have not investigated what changes, if any, would be necessary to run this in k8s.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/helm/charts/issues/9963#issuecomment-453126319, or mute the thread https://github.com/notifications/unsubscribe-auth/AKoi5gzR5Hr0EsMitnPykH97_PmFBvqAks5vB1ZDgaJpZM4ZRd4m .

donbowman commented 5 years ago

I too now have this issue. Any comments on your progress?

donbowman commented 5 years ago

I have created PR https://github.com/helm/charts/pull/11343 that could use some 3rd-party testing, but moves this to the official Zookeeper

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

fabriziofortino commented 5 years ago

waiting for #11343 to be reviewed / approved

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

donbowman commented 5 years ago

still waiting for review.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

donbowman commented 5 years ago

the review has been done on PR #11343 I'll make the changes

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

donbowman commented 5 years ago

will be closed by #11343

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

MartinX3 commented 5 years ago

Must stay open until #11343 got closed.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

agolomoodysaada commented 5 years ago

Still an issue

KealanM commented 5 years ago

16290 bumped Kafka chart zookeeper dependency to 2.0.0 from @donbowman so this should be resolved.