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: 🐛 load valuefiles from config file location #6156

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

ManAnRuck
Copy link
Contributor

What this PR does / why we need it:

fix read valueFiles in helm deployments with sources.repository.url

Which issue(s) this PR fixes:

Fixes #6146

Special notes for your reviewer:
It's same like #6147 just for valueFiles @stefreak


I did test it against this (new uncommitted) example:

show example

examples/remote-helm/garden.yml

apiVersion: garden.io/v1
kind: Project
name: remote-helm
environments:
  - name: local
    variables:
      baseHostname: remote-helm.local.demo.garden
providers:
  - name: local-kubernetes
    environments: [local]
variables:
  userId: ${kebabCase(local.username)}

examples/remote-helm/redis.garden.yml

kind: Deploy
description: Redis service for queueing votes before they are aggregated
type: helm
name: redis
varfiles:
  - ./.env
source:
  repository: 
    url: https://github.com/bitnami/charts#main
spec:
  chart:
    path: bitnami/redis
  valueFiles:
    - values.yml

examples/remote-helm/values.yml

auth:
  enabled: false

examples/remote-helm/.env

TEST=test

@ManAnRuck
Copy link
Contributor Author

@vvagaytsev @stefreak maybe u can also have a look here because it's nearly the same like #6147

@stefreak
Copy link
Member

stefreak commented Jun 11, 2024

@ManAnRuck thank you for the contribution!

Even though I understand it is confusing, I believe this one is slightly different than #6147, because the current code looks in the action.getBuildPath() explicitly, and changing it to action.effectiveConfigFileLocation() would potentially be a breaking change for existing users.

If we add test cases for the behaviour as well, I would be ok with a compromise: We first check if the valueFile exists in build path, and if yes we use it (to preserve the current behaviour) – if not, we fall back to looking for it at the Garden config file location.

In the next major version (0.14) we can add a breaking change to stop looking for valueFile existence in the build path.

WDYT?

CC @vvagaytsev

@ManAnRuck ManAnRuck force-pushed the helm-source-valuefiles branch from e5e9d01 to 01e1795 Compare June 11, 2024 14:07
@ManAnRuck
Copy link
Contributor Author

ManAnRuck commented Jun 11, 2024

yes I like this approach :)

I already add a check if the file exists in both directories and a more readable error if the file is not found.
for the test I maybe need some support.

Edit: in the last commit I remove throwing the error after pipeline fails when a file is not found 🫣
https://app.circleci.com/pipelines/github/garden-io/garden/25521/workflows/c0a3dbc3-cd03-4ff2-a0cc-1df31d26b5e8/jobs/528825?invite=true#step-113-30166_958
is the file maybe really missing 😅

@ManAnRuck ManAnRuck force-pushed the helm-source-valuefiles branch from 92327c4 to 2f89b6a Compare June 11, 2024 14:40
@vvagaytsev vvagaytsev requested a review from stefreak June 17, 2024 14:29
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

thanks for the contribution @ManAnRuck, and thank you for refactoring this to use the async file system function @vvagaytsev

This PR looks good to me, but what is missing is one test that cements this behaviour and mentioning this in the docs. With those two changes I'd be happy to merge this 👍

@ManAnRuck
Copy link
Contributor Author

ManAnRuck commented Jun 20, 2024

I think I need some help.

https://github.com/garden-io/garden/blob/main/docs/contributing/developing-garden.md#tests
(there are some typos which I can fix by another pr (npm run run e2e))

when I run npm run e2e I get the following error

Test action "e2e-tests" was not found. Available actions: e2e-tests-kaniko, e2e-tests-tasks, e2e-tests-variables, e2e-tests-vote-helm, e2e-tests-vote, e2e-tests-remote-sources and e2e-tests-demo-project

when I then run ./bin/garden test e2e-tests-kaniko

Error while copying from '/Users/.../garden/e2e/projects/code-synchronization' to '/Users/.../garden/.garden/build/e2e-tests/e2e/projects/code-synchronization': Source is neither a symbolic link, nor a file.

regarding the documentation, isn't the current description already the one that describes how it should actually work? i.e. put the files in the config instead of the build folder?

Note that the paths here should be relative to the config root, and the files should be contained in this action config's directory.
https://docs.garden.io/reference/module-types/helm#valuefiles

@ManAnRuck
Copy link
Contributor Author

@stefreak I added now an integration test. hope this is enough and we don't need e2e tests for this.

Manuel Ruck and others added 5 commits June 20, 2024 20:58
by checking getBuildPath first
and throws an error if both locations does not have the file

Signed-off-by: Manuel Ruck <[email protected]>
@ManAnRuck ManAnRuck force-pushed the helm-source-valuefiles branch from 85cc361 to 9d14a46 Compare June 20, 2024 18:58
@vvagaytsev vvagaytsev requested a review from stefreak June 21, 2024 10:41
@vvagaytsev
Copy link
Collaborator

Thanks for the update, @ManAnRuck! Feel free to create another PR to fix the testing guidelines! Huge thanks for pointing that out! 👍

The changes look good to me. @stefreak can you take a look too, please?

Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@stefreak stefreak added this pull request to the merge queue Jun 21, 2024
Merged via the queue into garden-io:main with commit 52dc2b1 Jun 21, 2024
17 checks passed
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.

Varfiles with "type: helm" and sources.repository.url not working
3 participants