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(template): helm template "--show-only" flag producing duplicates when flag used more than once #7204

Merged

Conversation

lbontecou
Copy link
Contributor

@lbontecou lbontecou commented Dec 10, 2019

Issue

#7203

Summary


  • While migrating from helm2 to helm3, we began switching --execute parameters to the new --show-only parameter that was listed as being equivalent.
  • We found our helm template <name> <chart> --show-only template1.yaml --show-only template2.yaml --values values.yaml | kubectl replace --force -f - commands failing due to "Resource already exists" errors.
  • Using grep, we were able to identify our issue, duplicated outputs when multiple --show-only were used.
$ helm template myapp myapp --show-only templates/job.yaml --values values.yaml | grep Source 
# Source: app/templates/job.yaml

$ helm template myapp myapp --show-only templates/configmap.yaml --values values.yaml | grep Source 
# Source: myapp/templates/configmap.yaml

$ helm template myapp myapp --show-only templates/configmap.yaml --show-only templates/job.yaml --values values.yaml  | grep Source   
# Source: myapp/templates/configmap.yaml
# Source: myapp/templates/configmap.yaml
# Source: myapp/templates/job.yaml

Solution


  • Identified an incorrect looping logic.
  • Tested using make and ./bin/helm
$ helm template myapp myapp --show-only templates/configmap.yaml --values values.yaml | grep Source 
# Source: myapp/templates/configmap.yaml

$ helm template myapp myapp --show-only templates/configmap.yaml --show-only templates/job.yaml --values values.yaml  | grep Source   
# Source: myapp/templates/configmap.yaml
# Source: myapp/templates/migrations-job.yaml

@helm-bot helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 10, 2019
@lbontecou lbontecou changed the title Bugfix : "helm template --show-only" producing duplicates #7203 - bugfix - "helm template --show-only" producing duplicates Dec 10, 2019
@lbontecou lbontecou force-pushed the bugfix/template-cmd-show-only-duplicates branch from 32cb1aa to df1a6d3 Compare December 10, 2019 23:34
Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
@lbontecou lbontecou force-pushed the bugfix/template-cmd-show-only-duplicates branch from df1a6d3 to 459a63c Compare December 10, 2019 23:36
@hickeyma hickeyma changed the title #7203 - bugfix - "helm template --show-only" producing duplicates fix(template): helm template "--show-only" flag producing duplicates when flag used more than once Dec 11, 2019
@hickeyma hickeyma added the bug Categorizes issue or PR as related to a bug. label Dec 11, 2019
@bacongobbler
Copy link
Member

Thanks! Would you mind writing some unit tests to cover this? Have a look at template_test.go for some examples. Let me know if you have any questions.

@bacongobbler
Copy link
Member

testdata output can be generated by running

$ cd cmd/helm # this is important; seems to only work when in the cmd/helm directory
$ go test -update

Run this once the unit test is created, and the testdata output will be generated for you.

Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

@hoboken515 Fix looks good, thanks for this. As suggested by @bacongobbler in #7204 (comment), I think unit test on this would be useful.

@lbontecou
Copy link
Contributor Author

I want to create the tests for this, may take me awhile to find the time / get up to speed on how to do so in the code base. Thanks for the inputs on how to do that @bacongobbler

@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 18, 2019
@lbontecou
Copy link
Contributor Author

@bacongobbler @hickeyma unittests added

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
@lbontecou lbontecou force-pushed the bugfix/template-cmd-show-only-duplicates branch from 197513c to 226a9a5 Compare December 18, 2019 22:06
Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
@hickeyma
Copy link
Contributor

@hoboken515 Can you rebase to fix the conflicts?

Lee Bontecou added 3 commits January 28, 2020 17:01
Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
@lbontecou lbontecou force-pushed the bugfix/template-cmd-show-only-duplicates branch from a478e1d to eb96f3f Compare January 28, 2020 23:08
@lbontecou
Copy link
Contributor Author

rebased

Copy link
Contributor

@hickeyma hickeyma 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 for catching and fixing this @hoboken515

@hickeyma hickeyma added needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. v3.x Issues and Pull Requests related to the major version v3 and removed awaiting review labels Jan 30, 2020
@hickeyma hickeyma added this to the 3.1.0 milestone Jan 30, 2020
@hickeyma hickeyma merged commit e483dce into helm:master Jan 30, 2020
bamachrn pushed a commit to bamachrn/helm that referenced this pull request Feb 28, 2020
…when flag used more than once (helm#7204)

* bugfix template show-only duplicates

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* 7203 - add unittests

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* attempt formatting fix

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* gofmt-ed with -s

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* goimports-ed with -local helm.sh/helm/v3 and gofmt-ed with -s -w

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* Update template_test.go

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* Update template_test.go

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
bamachrn pushed a commit to bamachrn/helm that referenced this pull request Feb 28, 2020
…when flag used more than once (helm#7204)

* bugfix template show-only duplicates

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* 7203 - add unittests

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* attempt formatting fix

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* gofmt-ed with -s

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* goimports-ed with -local helm.sh/helm/v3 and gofmt-ed with -s -w

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* Update template_test.go

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* Update template_test.go

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
bamachrn pushed a commit to bamachrn/helm that referenced this pull request Feb 28, 2020
…when flag used more than once (helm#7204)

* bugfix template show-only duplicates

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* 7203 - add unittests

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* attempt formatting fix

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* gofmt-ed with -s

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* goimports-ed with -local helm.sh/helm/v3 and gofmt-ed with -s -w

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* Update template_test.go

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>

* Update template_test.go

Signed-off-by: Lee Bontecou <lbontecou@thezebra.com>
@marckhouzam marckhouzam removed the needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. label Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. v3.x Issues and Pull Requests related to the major version v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants