Add sprig templating functions #559

tyrannasaurusbanks · 2017-04-19T15:35:23Z

The sprig library will give us some more functionality in the templates, and the next step would be to refactor the usage of the 2 custom template functions (minus and sha1) to make use of sprig implementations

k8s-ci-robot · 2017-04-19T15:35:26Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

codecov-io · 2017-04-19T16:08:20Z

Codecov Report

Merging #559 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #559   +/-   ##
======================================
  Coverage    37.7%   37.7%           
======================================
  Files          51      51           
  Lines        3302    3302           
======================================
  Hits         1245    1245           
  Misses       1854    1854           
  Partials      203     203

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7db8629...f7fe467. Read the comment docs.

mumoshu · 2017-04-20T00:52:05Z

glide.lock

 - name: gopkg.in/yaml.v2
-  version: a5b47d31c556af34a302ce5d659e6fea44d90de0
+  version: a83829b6f1293c91addabc89d0571c246397bbf4


go-yaml seems to be reverted to an older version here according to https://github.com/go-yaml/yaml/commits/v2
Did you intend to do so?

how do you add new dependency without re-resolving everything?

what I do:

add new dependency to "import" block in a source code where I need it

glide get <dependency>

git add vendor; git commit -a -m "added <depepndency>"

but this process still updates glide.lock and vendor even of unrelated packages.

@redbaron Yes this was the approach I used.
@mumoshu No i did not intend to pull in an older version - I'm quite surprised and a bit confused by this. Would you like me to revert the go-yaml version change?

Ah! If I remember correctly, although the gopkg.in is just a proxy for the repo in GitHub, the go-yaml v2 package served by gopkg.in has been a bit out-dated.
I believe I had manually modified the version after I ran glide update last time.

I guess it will be safe to rewrite the version manually back to a5b47d31c556af34a302ce5d659e6fea44d90de0 after you run glide get.

Other than that, your approach seems good to me.

Hm, sorry for the back and forth but even go-ini seems to be reverted to an older version
https://github.com/kubernetes-incubator/kube-aws/pull/559/files#diff-f16a80eae23d5b298c2652448ec420cfR53

I can't reproduce your result by just running glide get github.com/Masterminds/sprig.
Mine does change the version of go-yaml as expected but doesn't change the version of go-ini for example.

I've manually rewritten the version's back locally and it builds, so if you're happy with that then i suggest we stick with those:

- name: gopkg.in/yaml.v2 version: a5b47d31c556af34a302ce5d659e6fea44d90de0 - name: github.com/go-ini/ini version: 6e4869b434bd001f6983749881c7ead3545887d8

mumoshu · 2017-04-20T00:54:24Z

@tyrannasaurusbanks Thanks for the PR!
The eventual goal of refactoring templates sounds good to me.
One minor comment for now.
Would you mind confirming it?

tyrannasaurusbanks · 2017-04-20T15:34:42Z

No worries, great project, I thought it was about time I got on board and contributed, I'll refactor the templates after this one!

mumoshu · 2017-04-21T07:29:23Z

@redbaron @tyrannasaurusbanks Which versions of glide are you using?

For me:

$ glide --version
glide version 0.13.0-dev

tyrannasaurusbanks · 2017-04-21T10:27:00Z

I'm using the latest brew'ed glide:

$ glide --version
glide version 0.12.3

Where did you get 0.13.0-dev from, did you build it from source?

mumoshu · 2017-04-21T11:12:38Z

Where did you get 0.13.0-dev from, did you build it from source?

@tyrannasaurusbanks Thanks for the confirmation!
Yes - However I don't remeber the exact reason why I had done so.
According to the full changelog, I believe I don't need to stick with the dev version. Please let me confirm how it works for me with v0.12.3

mumoshu · 2017-04-21T11:20:17Z

@tyrannasaurusbanks Even with v0.12.3, It seems like I can't reproduce your result
https://gist.github.com/mumoshu/5e5ccec4e9c3b88637680411dd9d127a

Could you provide me your output of glide get?

tyrannasaurusbanks · 2017-04-21T12:50:10Z

Yeah sure, I've just reverted the changes on my local branch and re-added the dependency:
https://gist.github.com/tyrannasaurusbanks/b139abcc787aa2a5e20cc8c4926dc022

My colleagues each get a different version - how is this version hash created? I'd assumed it was a reference to a commit in the target-dependency's git repo - but this assumption is obviously wrong!

mumoshu · 2017-04-26T13:34:18Z

@tyrannasaurusbanks Thanks for the info!
Could you run cd ~/.glide/cache/src/https-gopkg.in-yaml.v2 && git branch and share me the commit id for the HEAD?

For me, it printed the commit id which equals to the one seen in the updated glide.yaml.

$ cd ~/.glide/cache/src/https-gopkg.in-yaml.v2
$ git branch
* (HEAD detached at a3f3340)
  v2

I guess that it prints a83829b for you?

The issue in glide which seemed relevant to me is: Masterminds/glide#592

mumoshu · 2017-04-26T13:53:32Z

@tyrannasaurusbanks I guess running glide cc & glide get github.com/Masterminds/sprig allows us to produce equivalent results.

Would you mind running it in your machine and confirming your result is equivalent to the one shown in my gist?
// Yours may contain newer commit ids if one or more dependencies are updated after this comment

tyrannasaurusbanks · 2017-05-02T15:36:36Z

@mumoshu yes, glide cc & glide get github.com/Masterminds/sprig has done the trick.

I've got some newer commit id's for github.com/aws/aws-sdk-go, but that makes sense given the updated timestamp in my log/run.
Thanks for taking the time to get to the bottom of this! I'll update my PR now

k8s-ci-robot · 2017-05-02T15:57:02Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

…ove unused 'sha1' function

mumoshu · 2017-05-08T00:20:27Z

filereader/texttemplate/texttemplate.go

@@ -13,12 +12,8 @@ func GetBytesBuffer(filename string, data interface{}) (*bytes.Buffer, error) {
 	if err != nil {
 		return nil, err
 	}
-	funcMap := template.FuncMap{
-		"sha1":  func(v string) string { return fmt.Sprintf("%x", sha1.Sum([]byte(v))) },


Seems like we used the sha1 func nowhere in templates. Good catch 👍

mumoshu · 2017-05-08T00:24:18Z

@tyrannasaurusbanks Great! Thanks a lot for taking your time to finish this 👍

tyrannasaurusbanks · 2017-05-15T08:49:44Z

@mumoshu no worries, thanks for taking the time to help me get my first commit merged - and for teaching me a bit about glide!
Hopefully I can start looking at some more meatier/impactful ones now!

…ture/add-sprig-templating-functions Add sprig templating functions

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 19, 2017

tyrannasaurusbanks force-pushed the feature/add-sprig-templating-functions branch from d97cb56 to 0cf7205 Compare April 19, 2017 15:47

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 19, 2017

tyrannasaurusbanks force-pushed the feature/add-sprig-templating-functions branch from 0cf7205 to 830179f Compare April 19, 2017 15:54

mumoshu reviewed Apr 20, 2017

View reviewed changes

tyrannasaurusbanks force-pushed the feature/add-sprig-templating-functions branch from 830179f to ecbc542 Compare May 2, 2017 15:56

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 2, 2017

Add sprig templating functions

70ab533

tyrannasaurusbanks force-pushed the feature/add-sprig-templating-functions branch from be4ee9f to 70ab533 Compare May 2, 2017 16:13

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 2, 2017

Replace custom 'minus' templating function with sprig-version and rem…

f7fe467

…ove unused 'sha1' function

tyrannasaurusbanks force-pushed the feature/add-sprig-templating-functions branch from bee950f to f7fe467 Compare May 3, 2017 15:12

mumoshu reviewed May 8, 2017

View reviewed changes

mumoshu merged commit 0cf3475 into kubernetes-retired:master May 8, 2017

mumoshu added this to the v0.9.7-rc.1 milestone May 8, 2017

kylehodgetts pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Mar 27, 2018

Merge pull request kubernetes-retired#559 from tyrannasaurusbanks/fea…

7efa2b9

…ture/add-sprig-templating-functions Add sprig templating functions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sprig templating functions #559

Add sprig templating functions #559

tyrannasaurusbanks commented Apr 19, 2017 •

edited

Loading

k8s-ci-robot commented Apr 19, 2017

codecov-io commented Apr 19, 2017 •

edited

Loading

mumoshu Apr 20, 2017

redbaron Apr 20, 2017

tyrannasaurusbanks Apr 20, 2017

mumoshu Apr 21, 2017

mumoshu Apr 21, 2017 •

edited

Loading

mumoshu Apr 21, 2017

mumoshu Apr 21, 2017

tyrannasaurusbanks Apr 21, 2017 •

edited

Loading

mumoshu commented Apr 20, 2017

tyrannasaurusbanks commented Apr 20, 2017

mumoshu commented Apr 21, 2017

tyrannasaurusbanks commented Apr 21, 2017 •

edited

Loading

mumoshu commented Apr 21, 2017

mumoshu commented Apr 21, 2017

tyrannasaurusbanks commented Apr 21, 2017 •

edited

Loading

mumoshu commented Apr 26, 2017 •

edited

Loading

mumoshu commented Apr 26, 2017 •

edited

Loading

tyrannasaurusbanks commented May 2, 2017

k8s-ci-robot commented May 2, 2017

mumoshu May 8, 2017

mumoshu commented May 8, 2017

tyrannasaurusbanks commented May 15, 2017 •

edited

Loading

Add sprig templating functions #559

Add sprig templating functions #559

Conversation

tyrannasaurusbanks commented Apr 19, 2017 • edited Loading

k8s-ci-robot commented Apr 19, 2017

codecov-io commented Apr 19, 2017 • edited Loading

Codecov Report

mumoshu Apr 20, 2017

Choose a reason for hiding this comment

redbaron Apr 20, 2017

Choose a reason for hiding this comment

tyrannasaurusbanks Apr 20, 2017

Choose a reason for hiding this comment

mumoshu Apr 21, 2017

Choose a reason for hiding this comment

mumoshu Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

mumoshu Apr 21, 2017

Choose a reason for hiding this comment

mumoshu Apr 21, 2017

Choose a reason for hiding this comment

tyrannasaurusbanks Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

mumoshu commented Apr 20, 2017

tyrannasaurusbanks commented Apr 20, 2017

mumoshu commented Apr 21, 2017

tyrannasaurusbanks commented Apr 21, 2017 • edited Loading

mumoshu commented Apr 21, 2017

mumoshu commented Apr 21, 2017

tyrannasaurusbanks commented Apr 21, 2017 • edited Loading

mumoshu commented Apr 26, 2017 • edited Loading

mumoshu commented Apr 26, 2017 • edited Loading

tyrannasaurusbanks commented May 2, 2017

k8s-ci-robot commented May 2, 2017

mumoshu May 8, 2017

Choose a reason for hiding this comment

mumoshu commented May 8, 2017

tyrannasaurusbanks commented May 15, 2017 • edited Loading

tyrannasaurusbanks commented Apr 19, 2017 •

edited

Loading

codecov-io commented Apr 19, 2017 •

edited

Loading

mumoshu Apr 21, 2017 •

edited

Loading

tyrannasaurusbanks Apr 21, 2017 •

edited

Loading

tyrannasaurusbanks commented Apr 21, 2017 •

edited

Loading

tyrannasaurusbanks commented Apr 21, 2017 •

edited

Loading

mumoshu commented Apr 26, 2017 •

edited

Loading

mumoshu commented Apr 26, 2017 •

edited

Loading

tyrannasaurusbanks commented May 15, 2017 •

edited

Loading