Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Workaround spf13/viper stringarray bug #1700

Merged
merged 3 commits into from
Feb 6, 2018

Conversation

carolynvs
Copy link
Contributor

@carolynvs carolynvs commented Jan 26, 2018

Fixes #1699.

  • Added failing test for the viper bug
  • Refactored our commands so that we can test validation errors

Let me know if you'd prefer that the last commit in this PR (which refactors the remaining commands to be testable), should be split into a separate PR.

Split up validation and command execution so that we can test them
separately
Switching from StringArray to StringSlice to workaround
spf13/viper#398.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 26, 2018
@MHBauer
Copy link
Contributor

MHBauer commented Jan 30, 2018

@jberkhahn Any opinion?

@MHBauer
Copy link
Contributor

MHBauer commented Jan 30, 2018

@carolynvs does the new testing catch the bug as it would have been?

@jberkhahn
Copy link
Contributor

I haven't dug into the Cobra stuff yet, so I can't comment on this. I will echo Morgan though, would like a test that shows this is fixed.

@carolynvs
Copy link
Contributor Author

The very first commit added a failing test. The second fixes it and the third commit added arg validation tests for the other commands.

Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

It is better to look at each commit individually rather than the final output.

LGTM

@MHBauer MHBauer added the LGTM1 label Feb 2, 2018
@jpeeler jpeeler added the LGTM2 label Feb 6, 2018
@jpeeler jpeeler merged commit fcdefa6 into kubernetes-retired:master Feb 6, 2018
@carolynvs carolynvs deleted the viper-stringarray-bug branch October 4, 2018 21:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants