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

refactor: remove vestiges of parsing release info text from helm deployment #6913

Merged

Conversation

briandealwis
Copy link
Member

This PR is a minor refactoring of the Helm post-deployment parsing to reflect the reality after #5723.

Prior to #5723, our Helm deployer would invoke helm get all <chart> to obtain the deployed manifests. This output some semi-structured text with release information, followed by the manifests. We discarded the release info and then parsed out the remaining YAML. But this could fail (#5484) as there was no clean separation between the release info and the YAML.

example showing output from `helm get all skaffold-helm`
$ helm get all skaffold-helm -n namespace
NAME: skaffold-helm
LAST DEPLOYED: Fri Nov 26 12:59:05 2021
NAMESPACE: namespace
STATUS: deployed
REVISION: 1
TEST SUITE: None
USER-SUPPLIED VALUES:
null

COMPUTED VALUES:
image: skaffold-helm:latest
replicaCount: 2

HOOKS:
MANIFEST:
apiVersion: apps/v1
kind: Deployment
metadata:
[...elided...]

Since we don't actually use the Helm release info text, #5723 changed our deployer to instead use helm get all <chart> --template {{.Release.Manifest}} to only output the Kubernetes manifests. But there were some vestiges of the release-info parsing still present.

This PR removes the last bits of this release-info parsing. In so doing, I discovered that our tests weren't providing an actual deployed manifest to helm get all, and so the namespaces were not being properly reported, and so our deploy-with-namespace tests weren't actually checking that the right namespaces were being reported!

@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #6913 (09ee40e) into main (290280e) will decrease coverage by 1.34%.
The diff coverage is 61.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6913      +/-   ##
==========================================
- Coverage   70.48%   69.14%   -1.35%     
==========================================
  Files         515      547      +32     
  Lines       23150    25080    +1930     
==========================================
+ Hits        16317    17341    +1024     
- Misses       5776     6575     +799     
- Partials     1057     1164     +107     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/flags.go 91.00% <0.00%> (+0.18%) ⬆️
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/fix.go 68.85% <40.00%> (-7.62%) ⬇️
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/skaffold.go 76.19% <70.00%> (-8.43%) ⬇️
... and 164 more

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 6e95e61...09ee40e. Read the comment docs.

Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

LGTM

@briandealwis briandealwis merged commit 2ae224e into GoogleContainerTools:main Nov 30, 2021
@briandealwis briandealwis deleted the rename-release-info branch November 30, 2021 16:32
briandealwis added a commit to briandealwis/skaffold that referenced this pull request Dec 9, 2021
tejal29 added a commit that referenced this pull request Jan 20, 2022
* refactor: remove vestiges of parsing release info text from helm deployment (#6913)

* refactor(testutil): allow tests to check for envvars that should not be set

* feat(helm): remove need for Helm deployer's artifactOverrides

* Back out changes to v1 schema

* pacify golangci-lint

* fix mac test and attempt to fix windows

* fix lint

Co-authored-by: tejal29 <tejal29@gmail.com>
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.

None yet

2 participants