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: handling varfiles in remote actions #6147

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

ManAnRuck
Copy link
Contributor

What this PR does / why we need it:

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

Which issue(s) this PR fixes:

Fixes #6146

Special notes for your reviewer:
It's just a dirty try to fix it. maybe there are some better solutions

@ManAnRuck ManAnRuck force-pushed the helm-source-varfiles branch from be50da1 to 6abf4c6 Compare June 5, 2024 17:50
@vvagaytsev vvagaytsev self-assigned this Jun 6, 2024
@vvagaytsev
Copy link
Collaborator

@ManAnRuck thank you so much for creating the PR! 👍

I'll take a closer look soon. Let's bring this together to the next release :)

@ManAnRuck
Copy link
Contributor Author

ManAnRuck commented Jun 6, 2024

i am currently thinking about what is a better solution.

  • trying to resolve varfiles and value files relative to original file
  • or copy varfiles and valuefiles into the .garden action directory

any suggestions or other ideas?

@ManAnRuck ManAnRuck changed the title fix: handling varfiles with helm sources WIP 🚧 fix: handling varfiles with helm sources Jun 6, 2024
@ManAnRuck ManAnRuck changed the title WIP 🚧 fix: handling varfiles with helm sources WIP: 🚧 fix: handling varfiles with helm sources Jun 6, 2024
@vvagaytsev vvagaytsev force-pushed the helm-source-varfiles branch from 5172cfc to a9e3457 Compare June 10, 2024 14:42
@vvagaytsev vvagaytsev changed the title WIP: 🚧 fix: handling varfiles with helm sources fix: handling varfiles in remote actions Jun 10, 2024
@vvagaytsev
Copy link
Collaborator

@ManAnRuck thanks for the fix! LGTM 👍

@stefreak @thsig can you take a look please? I've added a few more changes to the PR.

@vvagaytsev vvagaytsev requested review from stefreak and thsig June 10, 2024 15:01
@vvagaytsev vvagaytsev marked this pull request as ready for review June 10, 2024 15:01
@vvagaytsev vvagaytsev enabled auto-merge June 10, 2024 15:02
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 👍

@vvagaytsev vvagaytsev added this pull request to the merge queue Jun 10, 2024
Merged via the queue into garden-io:main with commit 4d9026f Jun 10, 2024
17 checks passed
@ManAnRuck
Copy link
Contributor Author

ManAnRuck commented Jun 10, 2024

Thank you :) then I will try now to reuse this function to get the valueFiles too 😃
#6156

@ManAnRuck ManAnRuck deleted the helm-source-varfiles branch June 10, 2024 17:01
@@ -5,6 +5,10 @@ source:
repository:
url: https://github.com/garden-io/garden-example-remote-module-jworker.git#main

# Do not delete, this must be resolved from the current path, not from the cloned sourced path
varfiles:
- ./.env
Copy link
Member

Choose a reason for hiding this comment

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

@vvagaytsev I just noticed this is an example, and not a test case– examples should be concise and this comment might confuse new users. Maybe it's better to create a more specific integ or unit test for this behaviour and remove this code from the example.

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