feat: Add support for image pull secrets in deployments #99
Conversation
🦋 Changeset detectedLatest commit: eb5384a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dhable
left a comment
dhable
left a comment
There was a problem hiding this comment.
Just a quick question on the unit tests
| clickhouse: | ||
| enabled: true | ||
| asserts: | ||
| - documentIndex: 0 |
There was a problem hiding this comment.
documentIndex works as long as we don't change the ordering of the documents in the file. Would a better pattern be to use documentSelector?
- it: should not include imagePullSecrets when not configured
set:
clickhouse:
enabled: true
documentSelector:
path: kind
value: Deployment
asserts:
- isNull:
path: spec.template.spec.imagePullSecrets
It looks like we're using documentIndex in the code so this matches. Just wondering if that's the pattern we want to use? It's more important when using the isNull check because a surprise reordering could still have a passing test because the test is working on a document that it didn't expect and thus gives a false positive - the key is missing because it's the wrong document, not because the template did the right thing.
There was a problem hiding this comment.
documentSelector is a better approach (The index reference already bit me once when I resolved a merge conflict with two different tests). I propose a widespread change / another ticket.
dhable
left a comment
dhable
left a comment
There was a problem hiding this comment.
I'm fine tweaking the unit tests in another ticket. 
Fixes: HDX-2198