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: Execute helm dependency build #154

Merged
merged 27 commits into from
Dec 29, 2023
Merged

Conversation

fritzduchardt
Copy link
Collaborator

Closes #146

Can be activated in app-data.ytt.yaml with:

helm:
  buildDependencies: true

@fritzduchardt fritzduchardt requested review from kbudde and Zebradil and removed request for kbudde December 23, 2023 19:43
@fritzduchardt fritzduchardt changed the title Execute helm dependency build feat: Execute helm dependency build Dec 23, 2023
Copy link
Member

@Zebradil Zebradil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug around a little and bumped into helm/helm#7214.

Can we just use helm template --dependency-update instead of manually downloading dependencies?

internal/myks/render.go Outdated Show resolved Hide resolved
internal/myks/util_test.go Outdated Show resolved Hide resolved
internal/myks/application.go Outdated Show resolved Hide resolved
@Zebradil
Copy link
Member

The flag seems to work perfectly. Here is how I tested:

cd $(mktemp -d)
cat <<EOF > Chart.yaml
apiVersion: v2
name: webserver
description: A Helm chart for Kubernetes
type: application
version: 0.1.0
appVersion: 1.16.0
dependencies:
  - name: mysql
    version: 9.3.4
    repository: https://charts.bitnami.com/bitnami
EOF
helm template . --dependency-update

Result:

image

@fritzduchardt
Copy link
Collaborator Author

I dug around a little and bumped into helm/helm#7214.

Can we just use helm template --dependency-update instead of manually downloading dependencies?

This is a pretty good find I was not ware of. Nevertheless, dependency build would still be my preference, since it respects the Chart.lock file. Dependency update would always renegotiate dependencies and lose the ability to pin Chart dependencies.

Copy link

codecov bot commented Dec 26, 2023

Codecov Report

Attention: 60 lines in your changes are missing coverage. Please review.

Comparison is base (4a3becc) 48.16% compared to head (e53d1ac) 47.54%.
Report is 7 commits behind head on dev.

Files Patch % Lines
internal/myks/render_helm.go 2.77% 34 Missing and 1 partial ⚠️
internal/myks/plugins.go 0.00% 21 Missing ⚠️
internal/myks/environment.go 33.33% 2 Missing ⚠️
cmd/root.go 0.00% 1 Missing ⚠️
internal/myks/bootstrap.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #154      +/-   ##
==========================================
- Coverage   48.16%   47.54%   -0.62%     
==========================================
  Files          26       26              
  Lines        2448     2509      +61     
==========================================
+ Hits         1179     1193      +14     
- Misses       1085     1131      +46     
- Partials      184      185       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Zebradil Zebradil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to isolate helm-related code in render_helm.go as much as possible. This will make navigation through the code easier.

internal/myks/application.go Outdated Show resolved Hide resolved
internal/myks/application.go Outdated Show resolved Hide resolved
@Zebradil
Copy link
Member

dependency build would still be my preference, since it respects the Chart.lock file.

That makes sense. I only wonder if the situations when there is a lock file but dependencies aren't built are frequent.

@Zebradil
Copy link
Member

I dug a bit deeper into helm and figured out another question: in which cases we would want to prioritize dependencies in Chart.lock over dependencies in Chart.yaml?

Here is an example Chart.lock with two dependencies:

dependencies:
- name: nginx
  repository: oci://registry-1.docker.io/bitnamicharts
  version: 14.2.2
- name: grafana
  repository: oci://registry-1.docker.io/bitnamicharts
  version: 9.6.5
digest: sha256:10d0bbd5014ddd04b24fa3311741326d6ddf992dff864b5a84b24bf564e3afbf
generated: "2023-12-26T13:33:52.347539414+01:00"

As you can see, dependencies don't carry any identification information apart from their versions. The digest field is used to verify that the Chart.yaml corresponds to the Chart.lock.

When I run helm template . --dependency-update, then change the digest manually and run helm template . --dependency-update again, nothing happens. When I run helm dependency update it detects the issue:

Error: the lock file (Chart.lock) is out of sync with the dependencies file (Chart.yaml). Please update the dependencies

So, helm dependency build is helpful in such a situation, but taking one step back: I still don't see in which circumstances --dependency-update won't work.

@fritzduchardt
Copy link
Collaborator Author

fritzduchardt commented Dec 27, 2023

So, helm dependency build is helpful in such a situation, but taking one step back: I still don't see in which circumstances --dependency-update won't work.

Good catch about oci registries, I should add that to the logic. Regarding the build vs update questions, have you considered that Helm also support semver wildcards in the Chart.yaml?

@Zebradil
Copy link
Member

Good catch about oci registries

Actually, in my tests, there was no need to add OCI registries separately. They just worked with helm dependency build.

have you considered that Helm also support semver wildcards in the Chart.yaml?

I didn't, but now I've checked that as well. I see that only helm dependency build installs the exact version from the lock file when semver pattern is used in the main config.

But can you maybe clarify your existing case a little more? In which case you'd have a lock file but no dependencies built? What's the workflow there?

@fritzduchardt
Copy link
Collaborator Author

fritzduchardt commented Dec 28, 2023

Good catch about oci registries

Actually, in my tests, there was no need to add OCI registries separately. They just worked with helm dependency build.

have you considered that Helm also support semver wildcards in the Chart.yaml?

I didn't, but now I've checked that as well. I see that only helm dependency build installs the exact version from the lock file when semver pattern is used in the main config.

But can you maybe clarify your existing case a little more? In which case you'd have a lock file but no dependencies built? What's the workflow there?

The workflow is as follows: we pull in a chart from git that is provided by another team. The chart is still in development and changes constantly. We use git instead of a helm registry, because we can create feature changes and tweak the chart. At the end we create a PR to submit the tweaks we require. This would have also have been a useful workflow for the platform components in the old days.

Thinking forward, we might want to support local charts as discussed during the last community meeting. This would also require correct support of Helms locking mechanism.

Copy link
Member

@Zebradil Zebradil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To move things forward, I reverted ArgoCD-related changes.

I'm still not sure if handling OCI registries the same way as usual HTTP registries is the best solution. As I wrote:

Actually, in my tests, there was no need to add OCI registries separately. They just worked with helm dependency build.

But if it works, I'm fine with that.

@fritzduchardt
Copy link
Collaborator Author

To move things forward, I reverted ArgoCD-related changes.

I'm still not sure if handling OCI registries the same way as usual HTTP registries is the best solution. As I wrote:

Actually, in my tests, there was no need to add OCI registries separately. They just worked with helm dependency build.

But if it works, I'm fine with that.

You are right. Oci registries don't require registration. I have taken them out again.

@fritzduchardt
Copy link
Collaborator Author

To move things forward, I reverted ArgoCD-related changes.

Thanks. Got lost a bit in the tangle of my open MR a while back.

@fritzduchardt fritzduchardt merged commit c2c0baa into dev Dec 29, 2023
5 of 6 checks passed
@fritzduchardt fritzduchardt deleted the execute-helm-dependency-build branch December 29, 2023 08:58
mykso-bot added a commit that referenced this pull request Dec 29, 2023
# [3.1.0](v3.0.4...v3.1.0) (2023-12-29)

### Bug Fixes

* **deps:** update golang.org/x/exp digest to 02704c9 ([#160](#160)) ([e9fe05e](e9fe05e))
* **plugins:** improve logging ([#190](#190)) ([ba31a3b](ba31a3b))
* **smart_mode:** add static files directory ([#193](#193)) ([3f52709](3f52709))

### Features

* Execute helm dependency build ([#154](#154)) ([c2c0baa](c2c0baa)), closes [#146](#146)
mykso-bot added a commit that referenced this pull request Dec 29, 2023
# [3.1.0](v3.0.4...v3.1.0) (2023-12-29)

### Bug Fixes

* **deps:** update golang.org/x/exp digest to 02704c9 ([#160](#160)) ([e9fe05e](e9fe05e))
* **plugins:** improve logging ([#190](#190)) ([ba31a3b](ba31a3b))
* **smart_mode:** add static files directory ([#193](#193)) ([3f52709](3f52709))

### Features

* Execute helm dependency build ([#154](#154)) ([c2c0baa](c2c0baa)), closes [#146](#146)
mykso-bot added a commit that referenced this pull request Dec 29, 2023
# [3.1.0](v3.0.4...v3.1.0) (2023-12-29)

### Bug Fixes

* **deps:** update golang.org/x/exp digest to 02704c9 ([#160](#160)) ([e9fe05e](e9fe05e))
* **plugins:** improve logging ([#190](#190)) ([ba31a3b](ba31a3b))
* **smart_mode:** add static files directory ([#193](#193)) ([3f52709](3f52709))

### Features

* Execute helm dependency build ([#154](#154)) ([c2c0baa](c2c0baa)), closes [#146](#146)
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 this pull request may close these issues.

[feat] templating of local charts with remote dependencies
2 participants