search

meilisearch / meilisearch-kubernetes

Meilisearch on Kubernetes Helm charts and manifests
https://www.meilisearch.com
MIT License
212 stars 59 forks source link

Add podAnnotations and expose some static values in values.yaml #58

Closed carlreid closed 3 years ago

carlreid commented 3 years ago

We need a way to annotate our pods for integration with other services, so this just adds support for that. Since I was updating these values, I thought I would also map some of those static values from the issue #53.

Resolves: #53

carlreid commented 3 years ago

I left a suggestion about the containerPort naming, I think it would be better to keep the same naming, to avoid confusion. WDYT?

Fine by me.

I wanted to add a little suggestion for other/next PRs and contributions (this is just my POV, but would like to know your opinion on it if you have something to add): I would rather separate different contributions into different PRs. For example, this one is having some documentation about how to use the charts, adding annotations to the charts, and introducing the static values on values.yaml from #53. This introduces 3 different changes and in case of problem, we can't simply revert one of them without affecting the others. It is a bit more work for both of us, but makes history more clear and useful, and reviews/changes simpler

I guess that's more for you guys to add to a contributors markdown with how you want to work/people to contribute. I'd have just treat the fix to the readme as a "boyscout" and then this other issue of #53 was also more of a quick fix because I was in this area for something that we just need (like another boyscout). It could be difficult to say exactly what is the maximum scope of a change to be exact though.

If we look at it that there wasn't an issue open and I had just made those changes anyway, would you have still preferred multiple PRs?

eskombro commented 3 years ago

I guess that's more for you guys to add to a contributors markdown with how you want to work/people to contribute.

True! I have been planning to do this for a while, I guess I'll do this now :)

I'd have just treat the fix to the readme as a "boyscout"

Thanks for the reference and the read! I like this principle!

If we look at it that there wasn't an issue open and I had just made those changes anyway, would you have still preferred multiple PRs?

Well, I think in this specific case it doesn't really make any huge difference, so this is why I mentioned it and ask for your opinion, but just to discuss and get another opinion sor perspectives. This is clearly not blocking anything to this PR :)

Thanks for accepting the suggestion, this looks ready to be merged!!!

eskombro commented 3 years ago

Bors merge

bors[bot] commented 3 years ago

Build succeeded: