Toolbox template #3287

gambol99 · 2017-08-26T15:51:18Z

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

allowing the users to use a snippets directory for reusable templates
allows the users to specify multiple templates files via multiple --template , use a directory or both
allows the users to specify multiple configuration files via multiple --values , use a directory or both
adding a safety check to ensure templates don't reference an unknown values
fixing the vetting issues to the method YamlToJson -> YAMLToJSON
as usual anything a saw on the journey which doesn't comply with go-vet got changed

Examples of a snippet

hooks:
  - name: some_service.service
    manifest: |
      {{ include "some_service.service" . | indent 6 }}

We currently use something similar to template our cluster and instances group documents, handling the differences between prod, ci and ephemeral

gambol99 · 2017-08-26T15:59:05Z

cc @mad01

gambol99 · 2017-08-27T12:28:24Z

/retest

chrislovecnm

Questions for you

chrislovecnm · 2017-08-28T00:35:12Z

pkg/util/file.go

+)
+
+// isDirectory checks if a path points to a directory
+func isDirectory(path string) (bool, error) {


We don't have any code like this or in or dependencies?

I've just taken a look at the util package in kubernetes but i can't anything that stands out .. I'll try again though

chrislovecnm · 2017-08-28T00:36:06Z

pkg/util/file.go

+}
+
+// ExpandFiles is responsible for resolving any references to directories
+func ExpandFiles(path string) ([]string, error) {


I think there is some code in kubectl that we should probably reuse. I can check tomorrow.

@justinsb thoughts?

same as above

chrislovecnm · 2017-08-28T00:39:14Z

pkg/util/render.go

@@ -0,0 +1,130 @@
+/*


Can we cut and actual package name for this? Maybe templater, you pick a name. Util pkg name drives me nuts.

chrislovecnm · 2017-08-28T00:42:06Z

pkg/util/render.go

+}
+
+// templateFuncsMap returns a map if the template functions for this template
+func (r *Renderer) templateFuncsMap(tm *template.Template) template.FuncMap {


Wonder if we should use a library for this. What is helm using?

@chrislovecnm Helm is based on text/template to be able to use gotemplate i guess. The helm templating code is going a bit more then just what the text/template package gives and were not that portable. i guess with some effort the helm templating can be a package but that is not without effort.

chrislovecnm

A couple of nit picks. Looks good overall. Wonder if we should use a lib like what helm uses for stuff like join and indent.

chrislovecnm · 2017-08-28T00:43:49Z

cmd/kops/toolbox_template.go

+	for _, x := range options.configPath {
+		list, err := putil.ExpandFiles(utils.ExpandPath(x))
+		if err != nil {
+			return err


Fmt error please

indeed! .. and fixed

chrislovecnm · 2017-08-28T00:44:20Z

cmd/kops/toolbox_template.go

+		list, err := putil.ExpandFiles(utils.ExpandPath(x))
+		if err != nil {
+			return err
+		}


Fmt? Helps me trace where the erro was returned

mad01 · 2017-08-28T09:12:35Z

@chrislovecnm @gambol99 for joins helm uses sprig might be a good idea to use the same to be able to have the same features as in helm

gambol99 · 2017-08-28T14:59:49Z

the sprig library is a good shout so i've updated to using that ... One question though how are managing the vendor directory? .. or you just copying in code? ... or i'm i missing something completely (admittedly i'm only familiar with godeps and glide) @chrislovecnm @justinsb

chrislovecnm · 2017-08-28T20:43:09Z

There are docs on how to vendor with the sub modules, you will probably want to do just a PR to add the vendoring. The docs are under dev folder in docs.

gambol99 · 2017-08-30T20:36:55Z

any other issues on this one? ...

chrislovecnm · 2017-08-31T00:47:19Z

@gambol99 sorry to be picky, but can we break the vendoring into a separate PR, and get that in first? Then we can get this code in. That is the workflow that we have followed in the past for new deps.

Thanks

justinsb · 2017-08-31T01:15:53Z

I previously looked at this and meant to apply lgtm - sorry for delay. I was able to follow the deps so I don't think these need to be split up...

/lgtm

k8s-github-robot · 2017-08-31T01:21:56Z

/test all [submit-queue is verifying that this PR is safe to merge]

gambol99 · 2017-08-31T10:54:44Z

/test pull-kops-e2e-kubernetes-aws

chrislovecnm · 2017-09-01T17:57:24Z

You may need another rebase to be e2e to pass. @justinsb I cannot remember when you put in the fix in kops that fixed e2e.

k8s-github-robot · 2017-09-02T15:22:46Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @gambol99 @justinsb

gambol99 · 2017-09-03T18:20:24Z

/test pull-kops-e2e-kubernetes-aws

chrislovecnm · 2017-09-04T15:33:16Z

/lgtm

k8s-github-robot · 2017-09-04T15:34:05Z

/test all [submit-queue is verifying that this PR is safe to merge]

chrislovecnm · 2017-09-04T15:35:23Z

Some more examples would be awesome, we should probably look at getting this out of toolbox ;)

k8s-github-robot · 2017-09-04T20:11:18Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @chrislovecnm @gambol99 @justinsb

chrislovecnm · 2017-09-06T23:23:20Z

@gambol99 sorry for the nit pick, but can you squash into appropriate chunks of work. I would like to have the vendoring commit moved to the first commit if you can. After that let's get it in ... I want to try it :)

@note

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

Adding the dependecies for the templating

gambol99 · 2017-09-08T20:27:26Z

/test pull-kops-e2e-kubernetes-aws

gambol99 · 2017-09-08T21:37:18Z

/test pull-kops-e2e-kubernetes-aws

chrislovecnm · 2017-09-10T20:30:45Z

/lgtm

k8s-github-robot · 2017-09-10T20:31:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, gambol99, justinsb

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

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

~~OWNERS~~ [chrislovecnm,justinsb]

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 · 2017-09-10T20:31:46Z

/test all [submit-queue is verifying that this PR is safe to merge]

chrislovecnm · 2017-09-10T20:32:46Z

@k8s-bot go bot go

@gambol99 we will need release notes please

k8s-github-robot · 2017-09-10T21:04:04Z

Automatic merge from submit-queue

chrislovecnm · 2017-09-10T21:14:33Z

Holy crap batman - it merged

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 26, 2017

gambol99 force-pushed the toolbox_template branch from c9af5c5 to f373335 Compare August 26, 2017 15:54

gambol99 force-pushed the toolbox_template branch from f373335 to e1ebdc6 Compare August 26, 2017 16:07

chrislovecnm reviewed Aug 28, 2017

View reviewed changes

chrislovecnm suggested changes Aug 28, 2017

View reviewed changes

gambol99 force-pushed the toolbox_template branch from e1ebdc6 to 94d1f7b Compare August 28, 2017 14:56

gambol99 force-pushed the toolbox_template branch from 94d1f7b to 91efafd Compare August 28, 2017 23:12

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 28, 2017

gambol99 force-pushed the toolbox_template branch from 91efafd to cf0425b Compare August 28, 2017 23:39

k8s-ci-robot assigned justinsb Aug 31, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017

gambol99 force-pushed the toolbox_template branch from cf0425b to 83c7397 Compare September 2, 2017 15:22

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2017

k8s-ci-robot assigned chrislovecnm Sep 4, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2017

gambol99 force-pushed the toolbox_template branch from 83c7397 to 2e1f424 Compare September 4, 2017 20:10

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2017

gambol99 closed this Sep 6, 2017

gambol99 reopened this Sep 6, 2017

gambol99 force-pushed the toolbox_template branch from 2e1f424 to 616860d Compare September 6, 2017 14:52

gambol99 force-pushed the toolbox_template branch from 616860d to 3d34292 Compare September 8, 2017 19:29

gambol99 added 2 commits September 8, 2017 20:30

Vendoring / Dependencies

404d940

Adding the dependecies for the templating

gambol99 force-pushed the toolbox_template branch from 3d34292 to 404d940 Compare September 8, 2017 19:30

chrislovecnm approved these changes Sep 10, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2017

k8s-github-robot merged commit 721ba52 into kubernetes:master Sep 10, 2017

gambol99 deleted the toolbox_template branch February 21, 2019 23:08

Toolbox template #3287

Toolbox template #3287

Conversation

gambol99 commented Aug 26, 2017 • edited Loading

gambol99 commented Aug 26, 2017

gambol99 commented Aug 27, 2017

chrislovecnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mad01 commented Aug 28, 2017 • edited Loading

gambol99 commented Aug 28, 2017 • edited Loading

chrislovecnm commented Aug 28, 2017

gambol99 commented Aug 30, 2017

chrislovecnm commented Aug 31, 2017

justinsb commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

gambol99 commented Aug 31, 2017

chrislovecnm commented Sep 1, 2017

k8s-github-robot commented Sep 2, 2017

gambol99 commented Sep 3, 2017

chrislovecnm commented Sep 4, 2017

k8s-github-robot commented Sep 4, 2017

chrislovecnm commented Sep 4, 2017

k8s-github-robot commented Sep 4, 2017

chrislovecnm commented Sep 6, 2017

gambol99 commented Sep 8, 2017

gambol99 commented Sep 8, 2017

chrislovecnm commented Sep 10, 2017

k8s-github-robot commented Sep 10, 2017

k8s-github-robot commented Sep 10, 2017

chrislovecnm commented Sep 10, 2017

k8s-github-robot commented Sep 10, 2017

chrislovecnm commented Sep 10, 2017

gambol99 commented Aug 26, 2017 •

edited

Loading

mad01 commented Aug 28, 2017 •

edited

Loading

gambol99 commented Aug 28, 2017 •

edited

Loading