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

Add kops toolbox template docs #3655

Merged
merged 1 commit into from
Oct 17, 2017

Conversation

kenden
Copy link
Contributor

@kenden kenden commented Oct 17, 2017

Document the template format of new command kops toolbox template (#3040)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 17, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @kenden. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 17, 2017
@kenden kenden force-pushed the add_template_doc branch 2 times, most recently from 6d246e5 to b1abcac Compare October 17, 2017 15:16
@gambol99
Copy link
Contributor

@kenden ... You just reminded me i probably need add the additional documentation related to PR aaf6143 ... which adds snippets, multiple templates / values, directories and sprig templates methods ..

@gambol99
Copy link
Contributor

LGTM

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Question for you

kind: InstanceGroup
metadata:
labels:
kops.k8s.io/cluster: {{.clusterName}}.{{.dnsZone}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that read a tab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@chrislovecnm
Copy link
Contributor

/ok-to-test
/assign

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 17, 2017
@chrislovecnm
Copy link
Contributor

@gambol99 we need release notes for SURE. @kenden do you mind doing another iteration on the docs?

@chrislovecnm
Copy link
Contributor

Holy crap I need to look at your git commit messages @gambol99

Toolbox Templating
Extending the current implementation of toolbox template to include multiple files and snippets. Note, i've removed the requirements for defaults as I think people should be forced to specifically pass them.

- fixing the vetting iseues to the method YamlToJson -> YAMLToJSON
- adding a safety check to ensure templates don't reference an unknown value
- extending the unit test to ensure the above works on main and snippets
- include the ability to specify multiple configuration files, useful for common.yaml and prod.yaml etc

Requested Changes - Toolbox Templating

Added the requested changes

- moved the templater into it's own package rather than using base util
- moved to using the sprig library for additional template function
- note: i couldn't find a native way in sprig to do snippets, also the i've overloaded the indent as it appears to do the indent on all lines rather than on the newline, meaning i'd have to shift my first line back by the indent to get it to work, which seems ugly

@chrislovecnm
Copy link
Contributor

/lgtm

@kenden If I can get you to do another PR on @gambol99 features docs please let me know, otherwise, I will shanghi @gambol99 :) It is always better when an ops user does the docs :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 38952a1 into kubernetes:master Oct 17, 2017
@gambol99
Copy link
Contributor

I'm gonna update the documentation to reflect features added by aaf6143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants