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

Add repo field to helm release #5410

Merged
merged 4 commits into from Mar 3, 2021

Conversation

bobby-richard
Copy link
Contributor

Fixes: #1665

Description
Currently, to use a a remote helm chart with skaffold, you must manually add the repository to your local helm config using helm repo add. In order to keep skaffold self contained, a repo field has been added to helm releases. If this field is set, the --repo argument will be passed to helm commands, allowing you to deploy remote helm charts without any additional helm configuration.

@bobby-richard bobby-richard requested a review from a team as a code owner February 17, 2021 20:14
@google-cla google-cla bot added the cla: yes label Feb 17, 2021
@bobby-richard bobby-richard changed the title Helm repo2 Add repo field to helm release Feb 17, 2021
@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #5410 (6d34770) into master (c1b3f76) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5410      +/-   ##
==========================================
+ Coverage   71.34%   71.38%   +0.03%     
==========================================
  Files         399      399              
  Lines       14508    14512       +4     
==========================================
+ Hits        10351    10359       +8     
+ Misses       3385     3383       -2     
+ Partials      772      770       -2     
Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 58.82% <ø> (ø)
pkg/skaffold/deploy/helm/args.go 75.00% <100.00%> (+0.97%) ⬆️
pkg/skaffold/deploy/helm/deploy.go 76.09% <100.00%> (+0.11%) ⬆️
pkg/skaffold/util/tar.go 56.00% <0.00%> (+5.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1b3f76...6d34770. Read the comment docs.

@bobby-richard-sed
Copy link

Any comments? Would love to get this merged.

@MarlonGamez
Copy link
Contributor

MarlonGamez commented Feb 24, 2021

Hey @bobby-richard, thanks for opening this, and sorry for taking a bit to respond. I'll be discussing this with the team shortly, I'll try to keep this updated with what we talk about. Do you think that we should simply remove the remote flag with this change? Since the repo field essentially tells us that it's remote and gives a bit more info?

@bobby-richard
Copy link
Contributor Author

Thanks @marlon-gamez. I thought about removing the remote property, but wasn't sure how to handle a breaking change to the schema. Also, if you remove the remote property, you lose the ability to specify the chartPath using the repo alias (though I'm not sure why you'd want to do that, if the repo field was available). I'm happy to take a stab at removing the remote field if you think that's the way to go.

@tejal29
Copy link
Member

tejal29 commented Feb 24, 2021

Thanks @marlon-gamez. I thought about removing the remote property, but wasn't sure how to handle a breaking change to the schema. Also, if you remove the remote property, you lose the ability to specify the chartPath using the repo alias (though I'm not sure why you'd want to do that, if the repo field was available). I'm happy to take a stab at removing the remote field if you think that's the way to go.

Thanks @bobby-richard & @marlon-gamez for bring the remote config discussion.

I looked more into the code to understand what the implication would be if we remove remote.
This config is used multiple places to detect remote charts and ignore file changes for dependencies etc in dev loop.

This change looks good to me and i will merge it when kokoro passes.

@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Feb 24, 2021
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Feb 24, 2021
@MarlonGamez MarlonGamez added the triage/duplicate This issue or pull request already exists label Feb 24, 2021
@bobby-richard
Copy link
Contributor Author

@tejal29 it looks like kokoro passed

@MarlonGamez
Copy link
Contributor

Hey @bobby-richard, sorry for the wait, we wanted to see if the author of #5193 had any ideas before we merge this. Thank you for the contribution! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes size/M triage/duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure other helm repository than the official helm hub
5 participants