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

fix(helmfile): make resolving deps in multi-doc files more stable #26078

Merged
merged 4 commits into from Dec 5, 2023

Conversation

pathob
Copy link
Contributor

@pathob pathob commented Dec 1, 2023

Changes

Renovate doesn't manage to resolve dependencies anymore when there is a YAML document separator between repositories and releases. This is achieved by not continue'ing anymore when the document doesn't contain a releases block. Repositories that have potentionally been discovered in a document before are overriden (same as YAML would do).

Context

The Renovate dependency resolution breaks when a Helmfile contains a document separator between the repositories and the releases blocks. Using a document separator can be necessary when working with environments, as the following example Helmfile output shows:

$ helmfile -e qa template
Warning: environments and releases cannot be defined within the same YAML part. Use --- to extract the environments into a dedicated part
Warning: environments and releases cannot be defined within the same YAML part. Use --- to extract the environments into a dedicated part
...

Since these blocks are lists, they can not be extended in later documents. so e.g. a repositories list defined in the first document would completely be overridden by a repositories list in a later document (that's normal YAML behaviour). The Renovate manager should be adjusted to mimic this behaviour and properly support multi-document Helmfiles.

The following simple example from the Helmfile documentation can be used for reproducing the issue.

Before working:

environments:
  qa:
  staging:
  prod:

## document separator to satisfy Helmfile:
---

repositories:
 - name: prometheus-community
   url: https://prometheus-community.github.io/helm-charts

releases:
- name: prom-norbac-ubuntu
  namespace: prometheus
  chart: prometheus-community/prometheus
  ## Add a version to the example
  version: 25.7.0
  set:
  - name: rbac.create
    value: false

Before not working:

environments:
  qa:
  staging:
  prod:

repositories:
 - name: prometheus-community
   url: https://prometheus-community.github.io/helm-charts

## document separator to satisfy Helmfile:
---

releases:
- name: prom-norbac-ubuntu
  namespace: prometheus
  chart: prometheus-community/prometheus
  ## Add a version to the example
  version: 25.7.0
  set:
  - name: rbac.create
    value: false

With this PR, both variants are now working.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

Closes #26077

@pathob pathob force-pushed the fix/26077-helmfile-multi-document branch from 8764338 to 81ebf1b Compare December 1, 2023 13:55
@pathob pathob marked this pull request as draft December 1, 2023 13:58
@pathob pathob force-pushed the fix/26077-helmfile-multi-document branch 2 times, most recently from dacc0fa to 2c45d37 Compare December 4, 2023 09:22
@pathob pathob marked this pull request as ready for review December 4, 2023 09:25
@pathob pathob force-pushed the fix/26077-helmfile-multi-document branch from 2c45d37 to 80ba2d0 Compare December 4, 2023 09:29
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Do we have any other cases where we parse multi-doc yaml files in a manager? I would like to compare the approaches in case there are any "gotchas" we've found elsewhere which should be ported here

@pathob
Copy link
Contributor Author

pathob commented Dec 4, 2023

Do we have any other cases where we parse multi-doc yaml files in a manager? I would like to compare the approaches in case there are any "gotchas" we've found elsewhere which should be ported here

@rarkins There are a few others, like Flux or Kubernetes, easy to find searching for parseYaml, but usually they work with YAML documents in a different way. For example Kubernetes expects a whole new manifest in each document, whereas in Helmfile these documents belong together.

@rarkins rarkins requested a review from secustor December 4, 2023 10:07
@rarkins
Copy link
Collaborator

rarkins commented Dec 4, 2023

Most uses of parseYaml are for non multi-docs

@HonkingGoose

This comment was marked as resolved.

@pathob
Copy link
Contributor Author

pathob commented Dec 4, 2023

@pathob Please do not force-push to your pull request branch. Read our contributing guide to learn more.

Yeah, I only did until the first review :)

@HonkingGoose

This comment was marked as resolved.

@rarkins rarkins requested a review from viceice December 4, 2023 16:39
rarkins
rarkins previously approved these changes Dec 4, 2023
lib/modules/manager/helmfile/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/helmfile/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/helmfile/extract.ts Outdated Show resolved Hide resolved
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

just a small test thing

lib/modules/manager/helmfile/extract.spec.ts Outdated Show resolved Hide resolved
@viceice viceice added this pull request to the merge queue Dec 5, 2023
Merged via the queue into renovatebot:main with commit 4c4bd3a Dec 5, 2023
36 checks passed
@pathob pathob deleted the fix/26077-helmfile-multi-document branch December 5, 2023 09:35
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 37.83.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants