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

Possible to set selector in parent helmfile? #344

Closed
2ZZ opened this issue Sep 15, 2018 · 22 comments · Fixed by #567
Closed

Possible to set selector in parent helmfile? #344

2ZZ opened this issue Sep 15, 2018 · 22 comments · Fixed by #567

Comments

@2ZZ
Copy link

2ZZ commented Sep 15, 2018

I have a helmfile that pulls in another helmfile
cluster1-helmfile.yaml

---
context: cluster1
helmfiles:
 - apps-helmfile.yaml

Inside the second helmfile there are selectors
apps-helmfile.yaml

releases:
- name: app1
  chart: repo/app1
  version: '*'
  labels:
    enabled: app1
- name: app2
  chart: repo/app2
  version: '*'
  labels:
    enabled: app2

Currently I deploy it using

helmfile --selector enabled=app1 -f cluster-helmfile.yaml sync

Instead is it possible to set the enabled label in the parent helmfile? Similar to #168 (comment)

@mumoshu
Copy link
Collaborator

mumoshu commented Sep 19, 2018

@2ZZ Yeah I see that there are use-cases like this!

What if we enhance helmfiles to accept more parameters per item, so that you could specify any supported options along with the name of sub-helmfile, like this:

helmfiles:
 - source: apps-helmfile.yaml
    selectors:
    - enabled=app1

@2ZZ
Copy link
Author

2ZZ commented Sep 19, 2018

that would be ideal for my use case 👍

@mumoshu
Copy link
Collaborator

mumoshu commented Sep 21, 2018

The implementation for this would rely on the same mechanism for #168. The difference is that we add an another path to specify default selectors that is used while running sub-helmfiles.

@2ZZ
Copy link
Author

2ZZ commented Sep 24, 2018

Hi @mumoshu , I just discovered the context setting in the parent helmfile is not inherited in the included helmfiles, is that a bug or intentional?

@mumoshu
Copy link
Collaborator

mumoshu commented Sep 26, 2018

@2ZZ That's intentional! I basically want each helmfile.yaml to be self-contained. That is, there should be no global state among parent and sub helmfiles.

@sgandon
Copy link
Contributor

sgandon commented Apr 6, 2019

I would really appreciate this feature too cause we are compositing helmfiles but we sometimes do not want to install everything in the sub-helmfile.
I like the proposal with selector below the sub-helmfile path.
In fact there could be a section for each of the global options of the helmfile cli such as environment, kube-context, log-level, selector, namespace, interactive, quiet

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 7, 2019

@2ZZ That's intentional! I basically want each helmfile.yaml to be self-contained. That is, there should be no global state among parent and sub helmfiles.

@sgandon Can we start by reverting my previous comment above, and just pass the context(label selector from the command-line) down to all the sub-helmfiles?

The rationale is that I want everyone to be able to split helmfile.yaml without tears when it gets huge enough.

The label selector applied to the parent helmfile only defeats this use-case. Because then, once you start splitting your helmfiles, you are unable to rely on label selector to selectively deploy releases anymore.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 7, 2019

@2ZZ Would you mind if I revert my comment and just make it pass label selector down to sub-helmfiles?

@sgandon
Copy link
Contributor

sgandon commented Apr 7, 2019

@mumoshu I am sorry I do not get what you are suggesting. But I think my use case is pretty similar to
@2ZZ .
Basically we have a 'team A' creating a set of charts and they are all gathered in a single helmfile.
And other teams want to use only a subset of this helmfile.
So the idea was to allow for other teams to pick a set of chart of 'teams A' using business labels selectors inside their own helmfiles.
In fact I believe if you allow inheritance like passing down commandline values to sub-helmfile, this creates more confusion and make is more complicated to understand what happens.
And when compositing helmfiles we really need a way to select what to deploy or not for sub-helmfile.

@sgandon
Copy link
Contributor

sgandon commented Apr 7, 2019

in my opinion the commandline should only impact the current helmfile for simplicity. That does not mean we can find a way to allow passing argument to sub-helmfile, but this should be done explicitly and not automatically.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 7, 2019 via email

@sgandon
Copy link
Contributor

sgandon commented Apr 8, 2019

I am not sure what you mean with env values if you are talking about environment variable then nope cause I want it to be specified in the parent helmfile, and if you mean helmfile environment values then why not, I don't see a real usage on the environment values right now for our use case.
I thought selectors where a good match as they seems to have been made for choosing what to deploy or not.
I also though including helmfile in an helmfile is just a way of launching multiple helmfile sub commands this is why I thought it would be great to propose helmfile cli global arguments in the parent helmfile definition something like that.

helmfiles:
 - ../common/all-common-svc-helmfile.yaml
    selectors:
    - enabled=app1
    - name=my-specific-release
    namespace: foo
    interactive: false
    kube-context: eks-dev

@sgandon
Copy link
Contributor

sgandon commented Apr 9, 2019

I believe this would match the desire of keeping the helmfile independent from each other in the sense that there is nothing more you can do using en composite helmfile that using multiple command lines.

@sgandon
Copy link
Contributor

sgandon commented Apr 21, 2019

In the end I think you should not leak selectors to subhelmfile cause this would mean you know all the labels of all the sub-helmfiles that are composed together.
To me the --environment is something that should be global but selector should only apply to the initially executed helmfile.
I was going to start the dev of this feature but I guess we should all agree on the selector behaviour.

@sgandon
Copy link
Contributor

sgandon commented Apr 21, 2019

the label of this issue is design finalized but since @mumoshu does not seem to agree we may want to remove it.

@sgandon
Copy link
Contributor

sgandon commented Apr 21, 2019

One possible way around this issue is to split the inital helmfile with multiple releases and labels to multiple helmfiles each for different set of releases. And this what we do right now, but then labels and selectors are no more helpfull and that is a shame cause you can cope with higher order of complexity using labels selector that you would with multiple helmfiles.

@sgandon
Copy link
Contributor

sgandon commented Apr 22, 2019

I am wondering if we could not provide both alternatives, that is have an inheritance of the selectors (either implicitly 'yucks', or explicitly) and also provide a way to specify them manually in the yaml.

  • so implicit which I don't like cause it is magical.
  • explicit with the possibility to specify those in the yaml like this
helmfiles:
 - apps-helmfile.yaml:
    selectors: inherited
 - apps1/*/helmfile.yaml:
    selectors:
    - enabled=app1
 - apps2/*/helmfile.yaml

This way you can have 3 different workflow, no selectors, inherited or manually set.
In fact I just realized while coding this that the implicit inheritance is already implemented and that is what @2ZZ is using. This is a pain cause it reduces flexibility and make things more "magical".

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 25, 2019

Ah, seems like I have been forgotten about that helmfile does passes selector down to sub-helmfiles! Thanks for reminding me about that.

For the meantime, I would like to just add selectors:. An empty selectors: can be used to prevent selectors from being inherited.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 25, 2019

...and that is to keep backward-compatibility. We can definitely revisit before helmfile reaching v1.0.

So, in short term, your example would turn into something like:

helmfiles:
 - apps-helmfile.yaml:
 - apps1/*/helmfile.yaml:
    selectors:
    - enabled=app1
 - apps2/*/helmfile.yaml
    selectors: {}

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 25, 2019

Another idea is, in addition to selectors:, introduce something like EXPERIMENTAL=true helmfile ... so that helmfile changes its behavior to "no selector inheritance by default".

@sgandon
Copy link
Contributor

sgandon commented Apr 25, 2019

I like all those ideas, I am currently developing it and hope to provide a PR soon.

@sgandon
Copy link
Contributor

sgandon commented Apr 27, 2019

@mumoshu in your latest example one case is missing or I did not get requirements right.
I have implemented the EXPERIMENTAL environment variable to not force inheritance but in the case we want inheritance what would you suggest in the yaml ?
I did suggest

helmfiles:
 - apps1/*/helmfile.yaml:
    selectors: inherited

but may there is another more yaml/elegant way to specify this ?

sgandon added a commit to sgandon/helmfile that referenced this issue Apr 27, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue Apr 29, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue Apr 29, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue Apr 30, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue May 4, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue May 4, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue May 4, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue May 4, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue May 4, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue May 4, 2019
mumoshu pushed a commit that referenced this issue May 5, 2019
Fixes #344 by allowing explicit selectors to be specified for composed helmfiles using the following structure

```yaml
helmfiles:
- path: helmfile.d/a*.yaml
  selectors:
  - name=prometheus      
  - name!=zipkin      
- helmfile.d/b*.yaml
- path: helmfile.d/c*.yaml
  selectors: {}
```

2 modes here : 
* legacy mode when no the env var HELMFILE_EXPERIMENTAL is not set to true
  * no selector : inherit from the command line.
  * selector:  is specified then it is used (an emty means no inheritance from command line and take everything).
* experimental when the env var HELMFILE_EXPERIMENTAL=true
  * no selector : nothing is inherited from the command line so use all releases.
  * selector:  is specified then it is used (an emty means no inheritance from command line and take everything).
sgandon added a commit to sgandon/helmfile that referenced this issue May 5, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue May 5, 2019
sgandon added a commit to sgandon/helmfile that referenced this issue May 5, 2019
mumoshu pushed a commit that referenced this issue May 6, 2019
…576)

Removed the usage of subhelmfile path as map key.
I also introduced the selectorsInherited key for explicit parent selector inheritance.

Ref #344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants