-
-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add environments values inheritance #741
base: main
Are you sure you want to change the base?
feat: add environments values inheritance #741
Conversation
47c0dc8
to
7b89699
Compare
7b89699
to
eaff5a1
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
f51532d
to
23d4ec7
Compare
f3a5e66
to
ec2a1b0
Compare
250e323
to
145b4da
Compare
Nice ! 💪 @mumoshu , with the additional I like this implementation as it looks like a fairly simple way to reduce boilerplate while avoiding |
145b4da
to
cee650a
Compare
@Vince-Chenal @hileef Hey! First of all, this looks awesome! I'm definitely looking forward to merging this with only a few changes described below. Can you confirm? My motivation for doing that is to make it work in concert with So what I'd like to do before merging this pull request is to change
This will result in the
This way, we can inject values "before the common layedValues derived from the template, but after all static values" via Not only each All in all, this design would give you all the benefits of environment templates along with layered values. Maximum reusability and DRY. WDYT? |
Hello @mumoshu , thanks for the feedback ! I've had a look at #840 and your summary proposal here, IMHO I think it's great 💪 I'll open a PR targetting this one shortly to add the change you requested, |
81d4476
to
61ab651
Compare
Hello again @mumoshu , it took me a while to have time to spend on this topic, ➡️ could you take a look and let us know if these updated changes fits your expectation ? |
1bec42f
to
e4bd518
Compare
4b20881
to
a058c98
Compare
@mumoshu ping |
@mumoshu anything we can do on our side to help move this forward ? 🙂 |
@mumoshu WDYT? |
Signed-off-by: Vincent Chenal <[email protected]>
a058c98
to
8edfda5
Compare
Refers to this discussion: #726
Summary
Allow inheriting values defined in previous values items.
There were some concerns about the risk of introducing a breaking change with this feature, that's why we changed the field name in order top isolate this behaviour and avoid breaking changes.
Default behaviour stays the same with environment values but you can now use
layeredValues
Example
We added a dedicated test for EnvironmentValuesLoader.
I'm not sure about the instantiation of EnvironmentValuesLoader within desiredStateLoader. Tell me if I should do it another way.