Configure affinity to be a variable to allow overriding #18
Conversation
There was a problem hiding this comment.
Thanks for putting this together! It will help folks be able to run on Minikube and support more complicated setups as well.
There are a couple of minor formatting fixes needed, but these changes should also have tests before we merge them. Specifically, it'd be good to test:
- The default defined values populate correctly, including indentation
- The null case creates correct and usable templates
|
Thanks I'll get on this in the next couple of days. |
Add test cases Improve variable commenting
|
@adilyse |
There was a problem hiding this comment.
Thank you for updating the PR! The code looks good and is ready to merge, but there are a few things that need to be changed in the tests. I'm happy to help you get them updated, or I can go ahead and make the changes and merge everything if you'd prefer.
I'd like to apologize for not having more information about writing tests for you before. I've written up some guidelines about the process here.
test/unit/server-statefulset.bats
Outdated
| --set 'server.affinity=' \ | ||
| . | tee /dev/stderr | | ||
| yq 'length > 0' | tee /dev/stderr) | ||
| [ "${actual}" = "true" ] |
There was a problem hiding this comment.
To match the changed condition above:
| [ "${actual}" = "true" ] | |
| [ "${actual}" = "false" ] |
* consul/master: (35 commits) Update CHANGELOG.md Pipe through the default flag for catalog sync Update to latest consul-k8s version update CHANGELOG to show connect injector RBAC support making serviceaccount consistent with other tests addressing feedback from review update CHANGELOG update CHANGELOG add test for mutatingwebhook namespace update CHANGELOG Rename files, add create to role fix: use a "-" instead of a ":" for names namespace is now set using helm release connectInject: adding tests, unique service account name per helm install Update CHANGELOG.md Add command continue indications where needed Update connect-inject-deployment.yaml Fix connect inject configuration name feat: create serviceAccount for sync catalog cluster role bind fix: move .Values.rbac.enabled to .Values.syncCatalog.rbac.enabled ...
|
Hi @adilyse Updated the indent to be cleaner |
AS per issue #13