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.

Sign up for GitHub

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

Jump to bottom

Add .spec.ignoreMissingValuesFiles to HelmChart API #1447

Merged
merged 7 commits into from
May 2, 2024
Merged

Add .spec.ignoreMissingValuesFiles to HelmChart API #1447

merged 7 commits into from
May 2, 2024

Conversation

isometry
Copy link
Contributor

This PR introduces the option to ignore missing values files referenced in generated HelmCharts. Full backward compatibility is maintained, and tests have been updated.

ArgoCD already has an equivalent option, and absence of this option has been a blocker for our adoption of FluxCD as we need to programmatically generate HelmRelease resources without knowledge of the particular override files available in each chart (whilst being aware that they follow deterministic naming conventions for different target environments).

I'll open a linked PR on fluxcd/helm-controller to expose the functionality upward.

@isometry isometry mentioned this pull request Apr 17, 2024
Merged
souleb
souleb reviewed Apr 17, 2024
View reviewed changes
docs/spec/v1beta1/helmcharts.md Outdated Show resolved Hide resolved
internal/helm/chart/builder.go Outdated Show resolved Hide resolved
internal/helm/chart/builder_local.go Outdated Show resolved Hide resolved
@isometry isometry mentioned this pull request Apr 17, 2024
Merged
@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 2728d26 to 3430651 Compare April 17, 2024 14:51
souleb
souleb reviewed Apr 17, 2024
View reviewed changes
api/v1beta1/helmchart_types.go Outdated Show resolved Hide resolved
internal/helm/chart/builder_local.go Show resolved Hide resolved
internal/helm/chart/builder_local.go Outdated Show resolved Hide resolved
@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 5eb8138 to 78cf64b Compare April 18, 2024 14:58
@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 78cf64b to 5be8a84 Compare April 19, 2024 16:57
@isometry
fix: gracefully handle PATH with whitespace
0fe6486 
Signed-off-by: Robin Breathe <robin@isometry.net>
@isometry isometry force-pushed the feature/ignore-missing-values-files branch 3 times, most recently from 7301e4e to 923bb3c Compare April 30, 2024 20:13
stefanprodan
stefanprodan reviewed Apr 30, 2024
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

Please undo all changes made to v1beta1 API and docs. The v1beta1 is deprecated and considered readonly. The new field should be documented in v1beta2 API doc.

isometry and others added 2 commits May 1, 2024 08:01
@isometry
feat(HelmChartSpec): optionally ignore missing valuesFiles
b84ab9e 
Signed-off-by: Robin Breathe <robin@isometry.net>
@pcanilho @isometry
feat: resolved values files are persisted to and retrieved from the […
bf97748 
…HelmChart.Status.ObservedValuesFiles] field

Signed-off-by: Paulo Canilho <paulo.canilho@nexthink.com>
@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 923bb3c to bf97748 Compare May 1, 2024 06:01
@isometry
Copy link
Contributor Author

isometry commented May 1, 2024

Please undo all changes made to v1beta1 API and docs. The v1beta1 is deprecated and considered readonly. The new field should be documented in v1beta2 API doc.

Done.

@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 5308956 to 1984abf Compare May 1, 2024 07:54
@isometry
doc: document ignoreMissingValuesFiles
b2702de 
Signed-off-by: Robin Breathe <robin@isometry.net>
@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 1984abf to b2702de Compare May 1, 2024 07:59
@stefanprodan
Copy link
Member

You need to regenerate the CRDs with make generate manifests and commit the changes.

@isometry
Copy link
Contributor Author

isometry commented May 1, 2024

You need to regenerate the CRDs with make generate manifests and commit the changes.

Sorry, failed to do this after the introduction of .status.observedValuesFiles. Done now.

@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 98b6cf3 to 4eb8a11 Compare May 1, 2024 10:30
@stefanprodan
Copy link
Member

Also docs, run make api-docs

@isometry
fix: regenerate manifests
6d96ae1 
Signed-off-by: Robin Breathe <robin@isometry.net>
@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 4eb8a11 to 6d96ae1 Compare May 1, 2024 10:34
@isometry
Copy link
Contributor Author

isometry commented May 1, 2024

Also docs, run make api-docs

Thank you. Done.

souleb
souleb reviewed May 1, 2024
View reviewed changes
internal/helm/chart/builder_local.go Outdated Show resolved Hide resolved
api/v1beta2/helmchart_types.go Outdated Show resolved Hide resolved
internal/helm/chart/builder.go Outdated Show resolved Hide resolved
internal/helm/chart/builder.go Outdated Show resolved Hide resolved
internal/controller/helmchart_controller.go Outdated Show resolved Hide resolved
internal/controller/helmchart_controller.go Outdated Show resolved Hide resolved
@isometry isometry force-pushed the feature/ignore-missing-values-files branch 2 times, most recently from 0ea6e37 to 51a9a2f Compare May 1, 2024 20:58
@isometry
chore: update tests
9b57d3b 
Signed-off-by: Robin Breathe <robin@isometry.net>
@isometry isometry force-pushed the feature/ignore-missing-values-files branch from 51a9a2f to 9b57d3b Compare May 2, 2024 06:21
@stefanprodan stefanprodan added area/helm Helm related issues and pull requests area/api API related issues and pull requests labels May 2, 2024
@stefanprodan stefanprodan changed the title Add .spec.ignoreMissingValuesFiles to HelmChartSpec Add .spec.ignoreMissingValuesFiles to HelmChart API May 2, 2024
souleb
souleb reviewed May 2, 2024
View reviewed changes
internal/controller/helmchart_controller.go Outdated Show resolved Hide resolved
internal/controller/helmchart_controller.go Outdated Show resolved Hide resolved
internal/helm/chart/builder_local.go Outdated Show resolved Hide resolved
internal/helm/chart/builder_remote.go Outdated Show resolved Hide resolved
@isometry isometry force-pushed the feature/ignore-missing-values-files branch 2 times, most recently from 6edb322 to a9b1ee6 Compare May 2, 2024 08:45
souleb
souleb reviewed May 2, 2024
View reviewed changes
internal/helm/chart/builder_local.go Outdated Show resolved Hide resolved
internal/helm/chart/builder_remote.go Outdated Show resolved Hide resolved
@isometry
chore: address feedback
1e82cec 
Signed-off-by: Robin Breathe <robin@isometry.net>
@isometry isometry force-pushed the feature/ignore-missing-values-files branch from a9b1ee6 to 1e82cec Compare May 2, 2024 10:10
@souleb
Copy link
Member

souleb commented May 2, 2024

I have tested the change in a kind cluster. It behaves as expected. See bellow:

  1. Helmchart without valuesFiles
Name:         podinfo
Namespace:    default
Annotations:  reconcile.fluxcd.io/requestedAt: 2024-05-02T11:45:05.090044+02:00
API Version:  source.toolkit.fluxcd.io/v1beta2
Kind:         HelmChart
Spec:
  Chart:               podinfo
  Interval:            5m0s
  Reconcile Strategy:  ChartVersion
  Source Ref:
    Kind:   HelmRepository
    Name:   podinfo
  Version:  ^6.0.x
Status:
  Artifact:
    Digest:            sha256:9db648076519fe8ae594182fe0bb74bceb3bd516407d5da6577216d12a1f6625
    Last Update Time:  2024-05-02T09:44:26Z
    Path:              helmchart/default/podinfo/podinfo-6.6.2.tgz
    Revision:          6.6.2
    Size:              14905
    URL:               http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/podinfo-6.6.2.tgz
...
  Last Handled Reconcile At:  2024-05-02T11:45:05.090044+02:00
  Observed Chart Name:        podinfo
  Observed Generation:        4
  URL:                        http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/latest.tar.gz
  1. Adding ValuesFiles (expect reconciliation error because of missing files)
...
Spec:
  Chart:               podinfo
  Interval:            5m0s
  Reconcile Strategy:  ChartVersion
  Source Ref:
    Kind:  HelmRepository
    Name:  podinfo
  Values Files:
    values.yaml
    values-production.yaml
  Version:  ^6.0.x
Status:
  Artifact:
    Digest:            sha256:9db648076519fe8ae594182fe0bb74bceb3bd516407d5da6577216d12a1f6625
    Last Update Time:  2024-05-02T09:44:26Z
    Path:              helmchart/default/podinfo/podinfo-6.6.2.tgz
    Revision:          6.6.2
    Size:              14905
    URL:               http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/podinfo-6.6.2.tgz
  Conditions:
    Last Transition Time:     2024-05-02T09:46:45Z
    Message:                  values files merge error: failed to merge chart values: no values file found at path 'values-production.yaml'
    Observed Generation:      5
    Reason:                   ValuesFilesError
    Status:                   True
    Type:                     Stalled
...
  Last Handled Reconcile At:  2024-05-02T11:45:05.090044+02:00
  Observed Chart Name:        podinfo
  Observed Generation:        5
  URL:                        http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/latest.tar.gz
Events:
  Type     Reason              Age                   From               Message
  ----     ------              ----                  ----               -------
  Warning  ValuesFilesError    1s                    source-controller  values files merge error: failed to merge chart values: no values file found at path 'values-production.yaml'
  1. Add IgnoreMissingFiles (expect new reconciled artifact and ObservedValuesFiles set)
...
Spec:
  Chart:                        podinfo
  Ignore Missing Values Files:  true
  Interval:                     5m0s
  Reconcile Strategy:           ChartVersion
  Source Ref:
    Kind:  HelmRepository
    Name:  podinfo
  Values Files:
    values.yaml
    values-production.yaml
  Version:  ^6.0.x
Status:
  Artifact:
    Digest:            sha256:e42702944dbc0425cef57fb84fdf1e1cb97ae0e3cad5dbabfe84614c85973aad
    Last Update Time:  2024-05-02T09:48:12Z
    Path:              helmchart/default/podinfo/podinfo-6.6.2+6.tgz
    Revision:          6.6.2+6
    Size:              14051
    URL:               http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/podinfo-6.6.2+6.tgz
  Conditions:
    Last Transition Time:     2024-05-02T09:48:12Z
    Message:                  packaged 'podinfo' chart with version '6.6.2+6' and merged values files [values.yaml]
    Observed Generation:      6
    Reason:                   ChartPackageSucceeded
    Status:                   True
    Type:                     Ready
    Last Transition Time:     2024-05-02T09:48:12Z
    Message:                  packaged 'podinfo' chart with version '6.6.2+6' and merged values files [values.yaml]
    Observed Generation:      6
    Reason:                   ChartPackageSucceeded
    Status:                   True
    Type:                     ArtifactInStorage
  Last Handled Reconcile At:  2024-05-02T11:45:05.090044+02:00
  Observed Chart Name:        podinfo
  Observed Generation:        6
  Observed Values Files:
    values.yaml
  URL:  http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/latest.tar.gz
Events:
  Type     Reason                 Age                   From               Message
  ----     ------                 ----                  ----               -------
  Warning  ValuesFilesError       87s                   source-controller  values files merge error: failed to merge chart values: no values file found at path 'values-production.yaml'
  Normal   ChartPackageSucceeded  0s                    source-controller  packaged 'podinfo' chart with version '6.6.2+6' and merged values files [values.yaml]
  1. Remove IgnoreMissingFiles (expect ObservedValuesFiles to match last reconciled artifact)
...
Spec:
  Chart:               podinfo
  Interval:            5m0s
  Reconcile Strategy:  ChartVersion
  Source Ref:
    Kind:  HelmRepository
    Name:  podinfo
  Values Files:
    values.yaml
    values-production.yaml
  Version:  ^6.0.x
Status:
  Artifact:
    Digest:            sha256:e42702944dbc0425cef57fb84fdf1e1cb97ae0e3cad5dbabfe84614c85973aad
    Last Update Time:  2024-05-02T09:48:12Z
    Path:              helmchart/default/podinfo/podinfo-6.6.2+6.tgz
    Revision:          6.6.2+6
    Size:              14051
    URL:               http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/podinfo-6.6.2+6.tgz
  Conditions:
    Last Transition Time:     2024-05-02T09:49:38Z
    Message:                  values files merge error: failed to merge chart values: no values file found at path 'values-production.yaml'
    Observed Generation:      7
    Reason:                   ValuesFilesError
    Status:                   True
    Type:                     Stalled
...
  Last Handled Reconcile At:  2024-05-02T11:45:05.090044+02:00
  Observed Chart Name:        podinfo
  Observed Generation:        7
  Observed Values Files:
    values.yaml
  URL:  http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/latest.tar.gz
Events:
  Type     Reason                      Age                    From               Message
  ----     ------                      ----                   ----               -------
  Warning  ValuesFilesError            14m (x2 over 15m)      source-controller  values files merge error: failed to merge chart values: no values file found at path 'values-production.yaml'
  Normal   ChartPullSucceeded          14m                    source-controller  pulled 'podinfo' chart with version '6.6.2'
  Normal   ArtifactUpToDate            8m55s (x5 over 34m)    source-controller  artifact up-to-date with remote revision: '6.6.2'
  Normal   ArtifactUpToDate            4m38s (x2 over 5m10s)  source-controller  artifact up-to-date with remote revision: '6.6.2'
  Normal   ChartPackageSucceeded       92s                    source-controller  packaged 'podinfo' chart with version '6.6.2+6' and merged values files [values.yaml]
  Normal   GarbageCollectionSucceeded  22s                    source-controller  garbage collected 1 artifacts
  Normal   ArtifactUpToDate            21s                    source-controller  artifact up-to-date with remote revision: '6.6.2+6'
  Warning  ValuesFilesError            6s (x2 over 2m59s)     source-controller  values files merge error: failed to merge chart values: no values file found at path 'values-production.yaml'
  1. Remove missing valuesFile (expect ObservedValuesFiles to be unset)
Spec:
  Chart:               podinfo
  Interval:            5m0s
  Reconcile Strategy:  ChartVersion
  Source Ref:
    Kind:  HelmRepository
    Name:  podinfo
  Values Files:
    values.yaml
  Version:  ^6.0.x
Status:
  Artifact:
    Digest:            sha256:9db648076519fe8ae594182fe0bb74bceb3bd516407d5da6577216d12a1f6625
    Last Update Time:  2024-05-02T09:51:09Z
    Path:              helmchart/default/podinfo/podinfo-6.6.2.tgz
    Revision:          6.6.2
    Size:              14905
    URL:               http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/podinfo-6.6.2.tgz
  Conditions:
    Last Transition Time:     2024-05-02T09:51:09Z
    Message:                  pulled 'podinfo' chart with version '6.6.2'
    Observed Generation:      8
    Reason:                   ChartPullSucceeded
    Status:                   True
    Type:                     Ready
    Last Transition Time:     2024-05-02T09:51:09Z
    Message:                  pulled 'podinfo' chart with version '6.6.2'
    Observed Generation:      8
    Reason:                   ChartPullSucceeded
    Status:                   True
    Type:                     ArtifactInStorage
  Last Handled Reconcile At:  2024-05-02T11:45:05.090044+02:00
  Observed Chart Name:        podinfo
  Observed Generation:        8
  URL:                        http://source-controller.helm-system.svc.cluster.local./helmchart/default/podinfo/latest.tar.gz
Events:
  Type     Reason                      Age                    From               Message
...

@stefanprodan
Copy link
Member

@souleb thanks for all the testing 🎖️

souleb
souleb approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@souleb souleb left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @isometry and @pcanilho for this contribution 🥇

stefanprodan
stefanprodan approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@stefanprodan stefanprodan merged commit 5fcae5c into fluxcd:main May 2, 2024
10 checks passed
@isometry
Copy link
Contributor Author

isometry commented May 2, 2024

Thank you, @souleb & @stefanprodan, for your support through this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcanilho pcanilho pcanilho left review comments

@stefanprodan stefanprodan stefanprodan approved these changes

@souleb souleb souleb approved these changes

Assignees
No one assigned
Labels
area/api API related issues and pull requests area/helm Helm related issues and pull requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@isometry @stefanprodan @souleb @pcanilho