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

don't show duplicate deployed releases (#63). add release name to the name output for helm (#67) #69

Merged
merged 10 commits into from
Apr 29, 2020

Conversation

lucasreed
Copy link
Contributor

also adds release name to output for helm

fixes #63
fixes #67

@lucasreed lucasreed requested a review from sudermanjr as a code owner April 27, 2020 19:19
@sudermanjr sudermanjr changed the title should not show duplicate deployed releases don't show duplicate deployed releases (#63). add release name to the name output for helm (#67) Apr 27, 2020
pkg/helm/helm.go Outdated Show resolved Hide resolved
pkg/helm/helm.go Outdated Show resolved Hide resolved
pkg/helm/helm.go Show resolved Hide resolved
Luke Reed and others added 2 commits April 27, 2020 16:05
Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com>
@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #69 into master will decrease coverage by 2.78%.
The diff coverage is 51.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   80.68%   77.90%   -2.79%     
==========================================
  Files           6        6              
  Lines         321      353      +32     
==========================================
+ Hits          259      275      +16     
- Misses         43       51       +8     
- Partials       19       27       +8     
Impacted Files Coverage Δ
pkg/helm/helm.go 62.96% <51.28%> (-8.47%) ⬇️

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 2497930...00e84e3. Read the comment docs.

@lucasreed
Copy link
Contributor Author

Hmm, I'm not sure how it calculated coverage went down by 3%. All the new things should be getting hit by the current tests. I can look tomorrow.

@lucasreed
Copy link
Contributor Author

Do you think it's worth adding a bunch of tests just to coverif err != nil lines?

@lucasreed lucasreed requested a review from sudermanjr April 29, 2020 14:39
@sudermanjr
Copy link
Member

Do you think it's worth adding a bunch of tests just to coverif err != nil lines?

Depends on whether you think those errors are important to test. If you don't, then I'm fine with the drop. Some of the marshal/unmarshal errors might be good to test, but I suspect it would be difficult to do and unlikely to hit that error case.

@lucasreed
Copy link
Contributor Author

lucasreed commented Apr 29, 2020

I think this is as good as it's going to get for now. We can definitely revisit this package in a separate PR and probably make it a bit more DRY which would help with test coverage.

Copy link
Member

@sudermanjr sudermanjr left a comment

Choose a reason for hiding this comment

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

Sounds like a plan. Thanks!

@sudermanjr sudermanjr merged commit 5b1de39 into master Apr 29, 2020
@sudermanjr sudermanjr deleted the lucasreed/duplicate_output_helm_chart branch April 29, 2020 19:57
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.

detect-helm and multiple uses of the same chart Only show latest DEPLOYED release when using helm
2 participants