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

fix a bug when rendering subcharts #381

Merged

Conversation

williepaul
Copy link
Contributor

  • in some cases, subcharts could not be rendered properly
  • change how we render values such that default capabilities are set, preventing rendering errors
  • set lintmode to true to allow ignoring of certain rendering errors from within the rendering engine
  • add resource count checking to load-dir unit tests
  • update error messages to make sure we are including the error data

 - in some cases, subcharts could not be rendered properly
- change how we render values such that default capabilities are set, preventing rendering errors
- set lintmode to true to allow ignoring of certain rendering errors from within the rendering engine
- add resource count checking to load-dir unit tests
- update error messages to make sure we are including the error data
@codecov
Copy link

codecov bot commented Nov 12, 2020

Codecov Report

Merging #381 (a5cc45f) into master (73d29aa) will increase coverage by 0.05%.
The diff coverage is 69.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   65.90%   65.95%   +0.05%     
==========================================
  Files          80       80              
  Lines        1830     1830              
==========================================
+ Hits         1206     1207       +1     
+ Misses        521      520       -1     
  Partials      103      103              
Impacted Files Coverage Δ
pkg/iac-providers/helm/v3/load-dir.go 85.61% <68.18%> (+0.68%) ⬆️
pkg/iac-providers/kubernetes/v1/normalize.go 74.46% <100.00%> (ø)

Copy link
Contributor

@devang-gaur devang-gaur left a comment

Choose a reason for hiding this comment

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

Minor changes requested. Also can you please change the Title ? I think its more than a bug fix ;)

pkg/iac-providers/helm/v3/load-dir.go Show resolved Hide resolved
pkg/iac-providers/helm/v3/load-dir.go Outdated Show resolved Hide resolved
return iacDocuments, chartMap, err
}

// for each template file found, render and save an iacDocument
var templateFileMap map[string][]*string
templateFileMap, err = utils.FindFilesBySuffix(filepath.Join(chartDir, helmTemplateDir), h.getHelmTemplateExtensions())
if err != nil {
logger.Warn("error while calling FindFilesBySuffix")
logger.Warn("error while calling FindFilesBySuffix", zap.Error(err))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.Warn("error while calling FindFilesBySuffix", zap.Error(err))
logger.Error("error while calling FindFilesBySuffix", zap.Error(err))

.project
.idea/
*.tmproj
.vscode/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add vendor/ here?

pkg/iac-providers/helm/v3/load-dir.go Outdated Show resolved Hide resolved
pkg/iac-providers/helm/v3/load-dir.go Outdated Show resolved Hide resolved
- errors were output when helm template rendering created a blank file, which is still valid yaml
- the "invalid kind" message is suppressed in this case, since rendering an empty template may be intentional
- error log levels and also error messages were updated per review comments
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kanchwala-yusuf kanchwala-yusuf merged commit 19f5527 into tenable:master Nov 13, 2020
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.

3 participants