kapp-controller
kapp-controller copied to clipboard
Added 'key' field in AppTemplateValuesSourceRef
What this PR does / why we need it:
Introduced a new optional field key
of type string
. This key can be used to reference specific instances of AppTemplateValuesSourceRef
in templates or configurations.
Which issue(s) this PR fixes:
Fixes #784
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
- [x] Follows the developer guidelines
- [ ] Relevant tests are added or updated
- [ ] Relevant docs in this repo added or updated
- [ ] Relevant carvel.dev docs added or updated in a separate PR and there's a link to that PR
- [x] Code is at least as readable and maintainable as it was before this change
Additional documentation e.g., Proposal, usage docs, etc.:
@jignyasamishra Thank you for looking into this issue. Are you still working on this? We would also need some logic to handle the value of that
Key
inAppTemplateValuesSourceRef
. Also, the tests will have to be updated accordingly. (There are some scripts in the hack directory that you can use to build and deploy kapp-controller, and run unit and integration tests. Please refer to docs directory for more details)
Thank you so much for your feedback!! Yes i am working on it.
Hello @praveenrewar I have addressed the changes and also changed the unit tests, the unit-tests are passing.Please have a look, thank you!
@jignyasamishra , Please check the tests. Once they are fixed, we will prioritize the review
Somewhat late to the game here, but some of my thoughts -- just spit-balling:
Would it be useful to make this new option a list, keys
?
I know I could add values from multiple keys by using multiple valuesFrom
with the same secret[^1], e.g.:
valuesFrom:
- secretRef:
name: my-secret
key: some-key
- secretRef:
name: my-secret
key: some-other-key
while this is what I am thinking about:
valuesFrom:
- secretRef:
name: my-secret
keys:
- some-key
- some-other-key
[^1]: same goes for config maps, as AppTemplateValuesSourceRef
is shared across those two
I know the related issue references prior art for picking up a secret holding a kube config, however I think these two concerns are slightly different: The kubeconfig part explicitly pulls out just one item of the secret (by default value
, we don't merge multiple items here, we only ever want to reference one), where using a secret as data-values, we generally want to get multiple, by default all, items.
@hoegaarden I think it makes sense to make keys a list. Thank you so much for the suggestion 😄 @jignyasamishra Are you still working on this? Let me know if you need some help or even if you are not able to continue working on this at the moment.
@hoegaarden I think it makes sense to make keys a list. Thank you so much for the suggestion 😄 @jignyasamishra Are you still working on this? Let me know if you need some help or even if you are not able to continue working on this at the moment.
sorry for the late reply @praveenrewar , I am occupied with my other work hence wont be able to work on this issue. Thank you and sorry for the inconvenience
No worries @jignyasamishra! I am closing this PR for now but feel free to reopen it if you want to get back to it.