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

Use Helm chart version in render #5922

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

briandealwis
Copy link
Member

Fixes: #5887
Merge after: #5921

Description
Use release version when rendering a remote chart.

User facing changes (remove if N/A)

  • skaffold render uses remote Helm chart version when specified

@briandealwis briandealwis requested a review from a team as a code owner May 30, 2021 03:26
@google-cla google-cla bot added the cla: yes label May 30, 2021
@codecov
Copy link

codecov bot commented May 30, 2021

Codecov Report

Merging #5922 (f3d0d05) into master (f7289a5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5922      +/-   ##
==========================================
+ Coverage   70.92%   70.94%   +0.01%     
==========================================
  Files         449      449              
  Lines       16981    16982       +1     
==========================================
+ Hits        12044    12048       +4     
+ Misses       4039     4038       -1     
+ Partials      898      896       -2     
Impacted Files Coverage Δ
pkg/skaffold/deploy/helm/deploy.go 77.62% <100.00%> (+0.10%) ⬆️
pkg/skaffold/docker/image.go 79.72% <0.00%> (+1.38%) ⬆️

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 f7289a5...f3d0d05. Read the comment docs.

Copy link
Contributor

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

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

LGTM

{
description: "deploy remote chart with version",
commands: testutil.
CmdRunWithOutput("helm version --client", version31).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: does helm require semver versions? I don't think so from:
https://helm.sh/docs/helm/helm_dependency/#synopsis

Copy link
Member Author

Choose a reason for hiding this comment

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

We require certain versions due to functionality being introduced. In this case, I just copied from the previous test.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Jun 1, 2021
@briandealwis briandealwis enabled auto-merge (squash) June 1, 2021 21:50
@briandealwis briandealwis merged commit 53b0780 into GoogleContainerTools:master Jun 1, 2021
@briandealwis briandealwis deleted the i5887 branch June 1, 2021 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

skaffold render ignores version with helm remoteChart
2 participants