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

Not installing dependancies when using selector #1692

Closed
bseenu opened this issue Feb 24, 2021 · 15 comments · Fixed by #1772
Closed

Not installing dependancies when using selector #1692

bseenu opened this issue Feb 24, 2021 · 15 comments · Fixed by #1772

Comments

@bseenu
Copy link

bseenu commented Feb 24, 2021

i have some thing like

releases:
  - name: cert-manager
    namespace: cert-manager
    chart: cert-manager/cert-manager
    version: v1.2.0
    missingFileHandler: Warn
  - name: prometheus
    needs:
      - cert-manager/cert-manager
    namespace: monitoring
    chart: prometheus/prometheus
    version: 13.3.1
    missingFileHandler: Warn
  - name: prometheus-adapter
    needs:
      - monitoring/prometheus
    namespace: monitoring
    chart: prometheus/prometheus-adapter
    version: 2.12.0
    missingFileHandler: Warn

And when i am using helmfile --selector name=prometheus apply , i expect cert-manager to be installed as dependency which is not happening

@roderik
Copy link
Contributor

roderik commented Feb 25, 2021

Same here, according to https://sweetops.slack.com/archives/CE5NGCB9Q/p1614102934006700 it should work

helmfile version v0.138.4

@bseenu
Copy link
Author

bseenu commented Feb 25, 2021

I am using the same version as well

@mwakkach
Copy link

I have a similar issue

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 10, 2021

@bseenu @roderik @mwakkach Hey everyone! Thanks for reporting.

Well, I wanted to say this was by-design.

My idea for --selector was to strictly select which releases to be processed, regardless of the processing order denoted by needs. So if you want cert-manager before prometheus as denoted by needs, you also need to list name=cert-manager in the selector. But yeah, this seems to be confusing to people.

That's not the end of the story though. Can I propose you all to change helmfile's behaviour around this to make it less confusing?

More concretely, I would suggest

  • helmfile -l name=prometheus apply to fail due to required release "cert-manager" is missing error

and

  • print informational log message to instruct you to either ...
    • add name=cert-manager to the selector and rerun helmfile apply.
    • or add a new flag --allow-missing-need to the command and rerun helmfile apply

This way, you can always notice that prometheus had the dependency to cert-manager and you can choose either to install or not to install the dependency along with prometheus.

WDYT?

@roderik
Copy link
Contributor

roderik commented Apr 10, 2021

We have been using "selector" as a definition of complex applications. e.g. AppA depends on Mongo, Rabbit and AppB depend on Rabbit and Postgres. We use helmfile programmatically (via shelling out the cli) but the calling app only knows about the application, not the app dependencies. So right now we are deploying all dependencies by default but this is a big waste.

@antaloala
Copy link

I think the new flag --allow-missing-need sounds like a really good proposal

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 10, 2021

@roderik Thanks for the info! I got to think that for your use-case we'd better add another flag like --include-need(s) so that helmfile -l name=prometheus --include-needs apply wouldn't fail and instead it installs cert-manager as well, automatically. Would that work for you?

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 10, 2021

@antaloala Hey! Thanks for the feedback. To be extra sure, could you also confirm that either you prefer, --allow-missing-need or --include-needs(#1692 (comment))?

My idea for --allow-missing-need was that it just skips installing cert-manager on installing helmfile -l name=prometheus apply, where helmfile -l name=prometheus --complete-needs installs both prometheus and cert-manager.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 11, 2021

Note that I've considered --complete-needs, --fulfill-needs as alternatives to --include-needs but it turned out the latest is more consistent with existing options in helm and helmfile like --include-crds.

Also note that I got to think --skip-needs is a more straightforward name than --allow-missing-need as we're going to have --skip-crds for helmfile via #1770.

mumoshu added a commit to variantdev/dag that referenced this issue Apr 11, 2021
This has been implemented originally to help roboll/helmfile#1692
@roderik
Copy link
Contributor

roderik commented Apr 11, 2021

I like the variety and terminology of Lerna's filter options: https://www.npmjs.com/package/@lerna/filter-options

@roderik
Copy link
Contributor

roderik commented Apr 11, 2021

Oh, missed a part of the question :) helmfile -l name=prometheus --include-needs apply would fit, but helmfile -l name=prometheus --include-dependencies apply sounds better :)

I realize you are trying to stay close to 'needs' but needing something makes you dependant on that thing, so I think it would still work.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 11, 2021

@roderik Thanks for the feedback!

I like the terminology used in Learn and I agree that --include-dependencies sounds better. I believe I generally have the same sentiment as yours 👍

To be clear, what's forcing me to stay with needs is that we already use the term dependencies in helmfile.yaml for adding dependent charts to the release:

releases:
- name: myapp
  chart: ./charts/mychart
  dependencies:
  - chart: ./charts/subchart
  needs:
  - anotherapp
- name: anotherapp
  chart: ./charts/anotherchart

In this context, to me, --include-dependencies and --skip-dependencies sounds like if you want to include/skip the charts myapp is dependent on.

@FWest98
Copy link

FWest98 commented Apr 11, 2021

I think sticking to needs is the better call here, as it more explicitly connects the helmfile field and CLI option. Overall, I think both --include-needs and --skip-needs are great additions to the package that I would greatly benefit from as well (I have a similar usecase to @roderik )

@antaloala
Copy link

I also vote for --include-needs and --skip-needs

@mumoshu
Copy link
Collaborator

mumoshu commented May 9, 2021

For backward-compatibility, I'm making skip-needs=true the default option in #1835. Please see the issues and comments linked from there for more context.

mumoshu added a commit that referenced this issue May 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants