Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Vince-Chenal
Copy link

@Vince-Chenal Vince-Chenal commented Mar 13, 2023

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

environments:
  default:
    layeredValues:
        - values1.yaml
        - values2.yaml.gotmpl <-- variables from values1.yaml are available in there

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.

@Vince-Chenal Vince-Chenal force-pushed the add-environments-values-inheritance branch from 47c0dc8 to 7b89699 Compare March 14, 2023 08:14
@Vince-Chenal Vince-Chenal force-pushed the add-environments-values-inheritance branch from 7b89699 to eaff5a1 Compare March 23, 2023 10:02
@stale
Copy link

stale bot commented Apr 6, 2023

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.

@stale stale bot added the wontfix This will not be worked on label Apr 6, 2023
@yxxhero yxxhero added feature in progress and removed wontfix This will not be worked on labels Apr 6, 2023
@Vince-Chenal Vince-Chenal force-pushed the add-environments-values-inheritance branch 2 times, most recently from f51532d to 23d4ec7 Compare May 4, 2023 12:40
@Vince-Chenal Vince-Chenal marked this pull request as ready for review May 4, 2023 15:09
@Vince-Chenal Vince-Chenal force-pushed the add-environments-values-inheritance branch 4 times, most recently from f3a5e66 to ec2a1b0 Compare May 12, 2023 07:22
@Vince-Chenal Vince-Chenal force-pushed the add-environments-values-inheritance branch 3 times, most recently from 250e323 to 145b4da Compare May 19, 2023 08:31
@hileef
Copy link

hileef commented May 19, 2023

Nice ! 💪 @mumoshu , with the additional layeredValues flag to guard against these changes being breaking,
do you think we could potentially include this before moving to a major version ? 🙂

I like this implementation as it looks like a fairly simple way to reduce boilerplate while avoiding exec & loadYAML machinery 🙏

@Vince-Chenal Vince-Chenal force-pushed the add-environments-values-inheritance branch from 145b4da to cee650a Compare May 25, 2023 13:02
@mumoshu
Copy link
Contributor

mumoshu commented May 27, 2023

@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 environmentTemplates and inherit feature proposed and discussed in #840 (comment).
Could you two read that thread, and my combined proposal below, and mind giving me feedbacks if any? 🙏

So what I'd like to do before merging this pull request is to change layedValues from a flag to another array of values.yaml files that can co-exist with the existing values so that it plays nicely with the environmentTemplates.

environmentTemplates:
  base:
    values:
    - common-static-values.yaml
    layeredValues:
    - common-dynamic-values-1.yaml
    - common-dynamic-values-2.yaml

environments:
  test:
    inherit:
      template: base
    values:
    - adhoc-static-values.yaml
    layeredValues:
    - adhoc-dynamic-values-1.yaml
    - adhoc-dynamic-values-2.yaml

This will result in the test environment to be:

environments:
  test:
    values:
    # (1) sees empty Values when rendering itself
    - common-static-values.yaml
    # (2) sees empty Values when rendering itself
    - adhoc-static-values.yaml
    layeredValues:
    # (3) sees (1)+(2)
    - common-dynamic-values-1.yaml
    # (4) sees (1)+(2)+(3)
    - common-dynamic-values-2.yaml
    # (5) sees (1)+(2)+(3)+(4)
    - adhoc-dynamic-values-1.yaml
    # (6) sees (1)+(2)+(3)+(4)+(5)
    - adhoc-dynamic-values-2.yaml

This way, we can inject values "before the common layedValues derived from the template, but after all static values" via adhoc-static-values.yaml. In other words, you can now pass inputs to the template-declared adhoc-dynamic-values-*.yaml files.

Not only each layedValues entry can see the latest merged values, the first layeredValues entry should see the values from common-static-values.yaml and adhoc-static-values.yaml for the input passing use-case I've described in the previous paragraph.

All in all, this design would give you all the benefits of environment templates along with layered values. Maximum reusability and DRY. WDYT?

@hileef
Copy link

hileef commented Jun 1, 2023

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 like that we can maintain the requested features here in a terse way,
while opening the door for supporting even more use cases ;

I'll open a PR targetting this one shortly to add the change you requested,
I'm pretty sure that @Vince-Chenal will be ok with approving it 🙂
Once done, we can probably re-update this PR and request your review for merge once again 🙏

@hileef
Copy link

hileef commented Jun 8, 2023

Hello again @mumoshu , it took me a while to have time to spend on this topic,
but we've now updated this PR to propose an implementation for the design as proposed in your above comment 🙂

➡️ could you take a look and let us know if these updated changes fits your expectation ?
cc. @Vince-Chenal keeping you in the loop

@hileef hileef force-pushed the add-environments-values-inheritance branch from 1bec42f to e4bd518 Compare June 17, 2023 12:42
@hileef
Copy link

hileef commented Jun 17, 2023

Hello @mumoshu , @yxxhero 👋
We've updated this PR to resolve the remaining conflicts against the main branch just now 🙏

It seems that you haven't had a chance to take a look at its contents yet,
could you let us know if there are things we can do in the meantime to help move this forward ? 🙂

@Vince-Chenal Vince-Chenal force-pushed the add-environments-values-inheritance branch from 4b20881 to a058c98 Compare June 29, 2023 07:42
@yxxhero
Copy link
Member

yxxhero commented Jun 29, 2023

@mumoshu ping

@hileef
Copy link

hileef commented Aug 25, 2023

@mumoshu anything we can do on our side to help move this forward ? 🙂

@stucki-stuck
Copy link

Hello,
We have exactly the same needs in our company. Any updates? @mumoshu or @yxxhero

@yxxhero
Copy link
Member

yxxhero commented Feb 11, 2024

@mumoshu WDYT?

Signed-off-by: Vincent Chenal <vincent.chenal@protonmail.com>
@Vince-Chenal Vince-Chenal force-pushed the add-environments-values-inheritance branch from a058c98 to 8edfda5 Compare February 15, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants