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 custom output and charts conflict error #938

Merged
merged 1 commit into from Feb 27, 2018

Conversation

hangyan
Copy link
Contributor

@hangyan hangyan commented Feb 9, 2018

fix #886

@cdrage I didn't find a correct way to add test for this change. So can you help me with this?

After this change, the logic now becomes:

assume there is a yaml in /tmp/a.yaml

  • -c : results in /tmp/a/
  • -c -o /tmp/charts : results in /tmp/charts/ (yaml in /tmp/charts/templates)
  • -o /tmp/charts: results in /tmp/charts
  • no args: results yaml in current dir

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 9, 2018
@cdrage
Copy link
Member

cdrage commented Feb 20, 2018

To be honest @hangyan I don't think there are any tests written at the moment for charts. I'll review this soon however and test it out before we merge this in!

@hangyan
Copy link
Contributor Author

hangyan commented Feb 24, 2018

@cdrage I have add some simple tests for charts and cutom output working together, I think it's enough for now

finalDirName = dirName + string(os.PathSeparator) + "templates"
}

log.Infof("YAML results in : %s", finalDirName)
Copy link
Member

Choose a reason for hiding this comment

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

Is this suppose to be debug information?

Copy link
Member

Choose a reason for hiding this comment

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

If not, it may be better to indicate that this is where the chart information will be :) For example: Chart files located at: %s", finalDirName or something along those lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdrage updated, remove the debug information, thanks very much!

@cdrage
Copy link
Member

cdrage commented Feb 27, 2018

Thanks so much for all these recent contributions! Tests pass and code LGTM 👍

@cdrage cdrage merged commit b365953 into kubernetes:master Feb 27, 2018
@hangyan
Copy link
Contributor Author

hangyan commented Feb 28, 2018

@cdrage Glad that I can help, hoping to do more contribute to this greate project

@hangyan hangyan deleted the fix-helm-custom-dir branch March 31, 2018 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output option with convert -c fails.
3 participants