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

[bitnami/milvus] feat: config external kafka tls client certs setting… #26118

Merged
merged 5 commits into from
Jun 6, 2024

Conversation

chenraoCR
Copy link
Contributor

@chenraoCR chenraoCR commented May 20, 2024

Description of the change

config externalKafka tls settings

Benefits

allow users to config kafka tls client certs when connect external kafka running with tls

Possible drawbacks

Applicable issues

Additional information

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@github-actions github-actions bot added milvus triage Triage is needed labels May 20, 2024
@github-actions github-actions bot requested a review from javsalgar May 20, 2024 09:23
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels May 20, 2024
@github-actions github-actions bot removed the triage Triage is needed label May 20, 2024
@github-actions github-actions bot removed the request for review from javsalgar May 20, 2024 09:46
@github-actions github-actions bot requested a review from migruiz4 May 20, 2024 09:46
Copy link
Member

@migruiz4 migruiz4 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @chenraoCR! I have updated the comments at milvus.prepareMilvusInitContainer to use capital letters.

Additionally, I would like to request one change in this PR.

Instead of {{- if (not (empty .Values.*)) }} could you please use {{- if .Values.* }}? That would simplify the conditionals and help improve code readability.

bitnami/milvus/templates/_helpers.tpl Outdated Show resolved Hide resolved
bitnami/milvus/templates/_helpers.tpl Outdated Show resolved Hide resolved
bitnami/milvus/templates/_helpers.tpl Outdated Show resolved Hide resolved
@chenraoCR
Copy link
Contributor Author

chenraoCR commented May 21, 2024

Thank you for your contribution @chenraoCR! I have updated the comments at milvus.prepareMilvusInitContainer to use capital letters.

Additionally, I would like to request one change in this PR.

Instead of {{- if (not (empty .Values.*)) }} could you please use {{- if .Values.* }}? That would simplify the conditionals and help improve code readability.

noted, and already updated

@chenraoCR
Copy link
Contributor Author

@migruiz4 i committed the PR for quite some time and updated the requested changes, could u please merge it?

@migruiz4
Copy link
Member

Hi @chenraoCR,

I'm sorry for the inconvenience. We were having an issue with the CI/CD pipeline for Milvus and the tests were not passing due to a problem unrelated to this PR.

Because the CI/CD pipeline would not succeed we couldn't merge this PR.

Signed-off-by: Bitnami Containers <bitnami-bot@vmware.com>
Signed-off-by: Bitnami Containers <bitnami-bot@vmware.com>
@chenraoCR
Copy link
Contributor Author

@migruiz4 now seems okay, plz check and merge, thanks

migruiz4 and others added 2 commits June 6, 2024 09:14
Signed-off-by: Miguel Ruiz <miruiz@vmware.com>
Signed-off-by: Bitnami Containers <bitnami-bot@vmware.com>
@migruiz4 migruiz4 merged commit 43e8796 into bitnami:main Jun 6, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
milvus solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants