lhotari commented on PR #438: URL: https://github.com/apache/pulsar-helm-chart/pull/438#issuecomment-1903370923
Thanks for the contribution @Mortom123 ! It looks like there's a lot of good improvements packaged here. > This pull request implements #426. It also moves some files around for overall better grouping of resources. > Additionally, the deployed pulsar-manager is conditioned with given secret credentials so the web login works out of the box. Please create individual PRs for separate changes. You don't have to create matching issues for each PR, but that can be done if that helps. > * moved files around This change is something that would be great to do, but it causes a significant drawback: it breaks diffs for Helm chart users that have modified their own Helm chart and keep it up-to-date by comparing their version to the upstream version. This is a common use case for Helm charts and that's why I'd like to push back on moving all files around. > * created an additional Batch Job which is along the lines of the `pulsar-cluster-initialize.yaml` but conditions the pulsar-manager, for this a new template function for the secret is introduced to keep it DRY. This is the main change for Pulsar Manager related changes? > * exposed the admin port of the pulsar-manager in its service I guess the admin port change could be a separate PR too > * renamed the initContainer resources for the nslookup container to better reflect where they are used This is a separate change so it should be handled in a separate PR. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
