roboll / helmfile

Deploy Kubernetes Helm Charts
MIT License
4.04k stars 566 forks source link

Repositories are not properly added in v0.125.1+ #1404

Open elordahl opened 4 years ago

elordahl commented 4 years ago

I've verified it's OK in v0.125.0. Output and file for v0.125.1 follow. What's strange is that if i rename the repository to stable, it is added.

output:
bash-5.0$ helmfile -v
helmfile version v0.125.1
bash-5.0$ helmfile repos
bash-5.0$
repositories.yaml:
repositories:
- name: traefik
  url: https://containous.github.io/traefik-helm-chart
- name: bitnami
  url: https://charts.bitnami.com/bitnami
mumoshu commented 4 years ago

@elordahl Hey! I've tried to reproduce it, but had no luck so far.

For me, it works. With v0.125.3 with the below helmfile.yaml:

repositories:
- name: traefik
  url: https://containous.github.io/traefik-helm-chart
- name: bitnami
  url: https://charts.bitnami.com/bitnami

and just ran

$ helmfile repos

After that, I see two repos exist in helm repo list output.

Would you mind giving me your helmfile.yaml, rather than repositories.yaml so that I can fully duplicate your environment?

elordahl commented 4 years ago

@mumoshu i dont have a single helmfile.yaml, but two files in the helmfile.d/ directory. do you want to see each of those files, or something else?

Thanks for the response --- btw, i've also tested v0.125.5 to no avail.

mumoshu commented 4 years ago

@elordahl Thanks for response! Yeah, sharing the two files for reproduction would be super helpful.

qqshfox commented 4 years ago

Same issue here. Set helmBinary in 1 of 2 helmfiles (helm2 for one, helm3 for another), and having same repo in both two helmfiles. Only one helm have the repo be added.

elordahl commented 4 years ago

Here are the files. Please note that i have also unsuccessfully tried with the addition of --- in various locations based on another thread.

helmfile.d/0.yaml:

bases:
  - ../repositories.yaml
  - ../environments.yaml

{{ readFile "../templates.yaml" }}

values:
  - ../environments/global.yaml

releases:
- name: init
  namespace: default
  chart: ../chart/init
  <<: *default

- name: loki-stack
  namespace: mesh
  chart: ../chart/loki-stack
  <<: *default

- name: traefik
  namespace: mesh
  chart: ../chart/traefik
  <<: *default

helmfile.d/1.yaml:

bases:
  - ../repositories.yaml
  - ../environments.yaml

{{ readFile "../templates.yaml" }}

values:
  - ../environments/security.yaml

releases:
- name: influxdb
  namespace: app
  disableValidation: true
  chart: ../chart/influxdb
  <<: *default
Nuru commented 4 years ago

Running into the same or similar problem in v0.125.0 through 0.125.5:

Repo defined in repositories but not used in releases, but referenced by a chart dependency, is not added by helmfile repos or helmfile deps.

Probably needs a fix like #1408 but for helm git

Nuru commented 4 years ago
helmfile.yaml ```yaml repositories: # Cloud Posse incubator repo of helm charts - name: "cloudposse-incubator" url: "https://charts.cloudposse.com/incubator/" # Incubator repo of official helm charts (used by monochart) - name: "kubernetes-incubator" url: "https://kubernetes-charts-incubator.storage.googleapis.com/" releases: - name: 'example' chart: "cloudposse-incubator/monochart" version: "0.22.0" values: - fullnameOverride: "infrastructure-docs" ```

Expected:

$ helmfile version
helmfile version v0.124.0
$ helmfile deps
Adding repo cloudposse-incubator https://charts.cloudposse.com/incubator/
"cloudposse-incubator" has been added to your repositories

Adding repo kubernetes-incubator https://kubernetes-charts-incubator.storage.googleapis.com/
"kubernetes-incubator" has been added to your repositories

Updating repo
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "cloudposse-incubator" chart repository
...Successfully got an update from the "kubernetes-incubator" chart repository
Update Complete. ⎈ Happy Helming!⎈ 

Updating dependency /tmp/130468758
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "kubernetes-incubator" chart repository
...Successfully got an update from the "cloudposse-incubator" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading monochart from repo https://charts.cloudposse.com/incubator/
Deleting outdated charts

Got:

$ helmfile version
helmfile version v0.125.5
$ helmfile deps
Adding repo cloudposse-incubator https://charts.cloudposse.com/incubator/
"cloudposse-incubator" has been added to your repositories

Updating dependency /tmp/381431663
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "cloudposse-incubator" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading monochart from repo https://charts.cloudposse.com/incubator/
Deleting outdated charts
mumoshu commented 4 years ago

@elordahl Thanks. I'll see if I can reproduce it.

@Nuru Thanks! Good point - I have not considered about that case. We will probably need to revert half of #1383, so that we don't filter repositories to update by releases

@qqshfox Thanks! I think Helmfile has never worked correctly in your case. Helmfile's never taken helmBinary into account when deciding whether to skip a repo update or not.

So I'm not yet sure what to do for @elodani's case. But the general direction would be I'll revert half of #1383, so that Helmfile doesn't filter repos to update by releases that has direct dependencies to the repos. It's basically pre 0.125.0 behaviour. In addition to that, I'd add helmBinary into account when computing the cache key for repository updates, which would make @qqshfox's case consistently work.

Nuru commented 4 years ago

@Nuru Thanks! Good point - I have not considered about that case. We will probably need to revert half of #1383, so that we don't filter repositories to update by releases

I like filtering repositories, and believe it is still an open request to avoid multiple redundant repo updates when processing a composite helmfile that references the same repo multiple times. It's just that you need to include repos that are reached via chart dependencies (requirements.yaml, etc.). I would prefer it if I did not have to declare those repos in the helmfile.yaml at all, since they are subject to change by the chart author.

mumoshu commented 4 years ago

I like filtering repositories, and believe it is still an open request to avoid multiple redundant repo updates when processing a composite helmfile that references the same repo multiple times.

Yeah, that's the pre-0.125.0 behaviour. It worked by "remember"ing the already updated repo, so that Helmfile doesn't duplicate updates to a repo.

mumoshu commented 4 years ago

It's just that you need to include repos that are reached via chart dependencies (requirements.yaml, etc.). I would prefer it if I did not have to declare those repos in the helmfile.yaml at all, since they are subject to change by the chart author.

I guess it won't worth the effort. But it does look like a valid feature request itself! Would you mind opening a dedicated issue for that?

elordahl commented 4 years ago

thank you, @mumoshu !