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

Helm flags for Global, Install and Upgrade helm commands #1673

Merged
merged 8 commits into from Feb 26, 2019

Conversation

tjerkw
Copy link

@tjerkw tjerkw commented Feb 19, 2019

This solves the following outstanding pull requests (they can all be closed):
#1284 #1506 #1507 #1445 #1499

It allows configuring flags to helm, for

The code is similar to the code in kustomize, and kubectl. They all have the option to add flags to the deploy commands.

This also makes skaffold easier to use if helm comes with new flags, it will "just work". Since the flags are free text. Just add them, and run 👍

Btw: The flags are set on the top level "HelmDeploy" struct. Not in the "HelmRelease" section in the yaml file.

This is because a "Global" flags list doesn't make sense per release. Then its not global anymore.

Custom flags to "helm install/upgrade" do make sense per release. But most of the time you probably want to keep them similar. Thats why i didn't set the flag on the HelmRelease struct.

Also: The flags clash a bit with the "Wait" and "RecreatePods" and "Namespace" fields in HelmRelease. However if you use both helm would complain and not run. So we're fine there.

@tjerkw tjerkw changed the title Helm flags for Global, Install and Update helm commands Helm flags for Global, Install and Upgrade helm commands Feb 19, 2019
Copy link
Contributor

@priyawadhwa priyawadhwa 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 your contribution! Left some comments.

integration/examples/annotated-skaffold.yaml Outdated Show resolved Hide resolved
pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved
pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved
integration/examples/annotated-skaffold.yaml Outdated Show resolved Hide resolved
examples/annotated-skaffold.yaml Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 19, 2019

Codecov Report

Merging #1673 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1673      +/-   ##
==========================================
+ Coverage   46.52%   46.55%   +0.02%     
==========================================
  Files         125      125              
  Lines        5661     5664       +3     
==========================================
+ Hits         2634     2637       +3     
  Misses       2754     2754              
  Partials      273      273
Impacted Files Coverage Δ
pkg/skaffold/deploy/helm.go 61.96% <100%> (+0.49%) ⬆️

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 85f46a7...3c34f41. Read the comment docs.

@tjerkw
Copy link
Author

tjerkw commented Feb 19, 2019

This is now ready to go. Thanks for the comments @priyawadhwa

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

One more fix :)

pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved
@priyawadhwa priyawadhwa added the kokoro:run runs the kokoro jobs on a PR label Feb 20, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Feb 20, 2019
@tjerkw
Copy link
Author

tjerkw commented Feb 25, 2019

Is this ok to go?

@priyawadhwa
Copy link
Contributor

@tjerkw I think you just need to rebase, and then I can merge this!

@priyawadhwa
Copy link
Contributor

Looks like travis failed --

--- FAIL: TestSchemas (0.26s)
	main_test.go:30: json schema files are not up to date.
           Please run `make generate-schemas` and commit the changes.

If you run that command and commit I think that should fix it!

@tjerkw
Copy link
Author

tjerkw commented Feb 26, 2019

Done. Forgot about it.

@priyawadhwa priyawadhwa added the kokoro:run runs the kokoro jobs on a PR label Feb 26, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Feb 26, 2019
Copy link
Contributor

@priyawadhwa priyawadhwa 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 so much for your contribution!

@priyawadhwa priyawadhwa merged commit b64a0ed into GoogleContainerTools:master Feb 26, 2019
@tjerkw tjerkw deleted the feature/helm-args branch February 27, 2019 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants