kapp-controller icon indicating copy to clipboard operation
kapp-controller copied to clipboard

Added 'key' field in AppTemplateValuesSourceRef

Open jignyasamishra opened this issue 1 year ago • 4 comments

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 avatar Feb 01 '24 12:02 jignyasamishra

@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 in AppTemplateValuesSourceRef. 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.

jignyasamishra avatar Feb 06 '24 11:02 jignyasamishra

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 avatar Feb 11 '24 19:02 jignyasamishra

@jignyasamishra , Please check the tests. Once they are fixed, we will prioritize the review

renuy avatar Mar 05 '24 06:03 renuy

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 avatar Mar 06 '24 10:03 hoegaarden

@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.

praveenrewar avatar Apr 05 '24 21:04 praveenrewar

@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

jignyasamishra avatar Apr 07 '24 12:04 jignyasamishra

No worries @jignyasamishra! I am closing this PR for now but feel free to reopen it if you want to get back to it.

praveenrewar avatar Apr 08 '24 22:04 praveenrewar