Inject stack name into userdata for nodepool workers #735
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
Codecov Report
@@ Coverage Diff @@
## master #735 +/- ##
=======================================
Coverage 34.33% 34.33%
=======================================
Files 55 55
Lines 3073 3073
=======================================
Hits 1055 1055
Misses 1859 1859
Partials 159 159Continue to review full report at Codecov.
|
|
Whole UserData refactor was done as a preparation for Ingition |
| @@ -21,9 +22,27 @@ func Parse(filename string, funcs template.FuncMap) (*template.Template, error) | |||
| } | |||
| return content, nil | |||
| }, | |||
| "toJSON": func(v interface{}) (string, error) { | |||
There was a problem hiding this comment.
toJson is already available through Masterminds/sprig according to https://github.com/Masterminds/sprig/blob/dba49a8d3a46ae8522f79e2c0241842d6ab613ca/defaults.go#L66
Could we use the one from sprig instead?
There was a problem hiding this comment.
I didn't check if it exists in a code, because it is undocumented on their godoc page: https://godoc.org/github.com/Masterminds/sprig
so I am not sure we can rely on it being available all the time
There was a problem hiding this comment.
@redbaron I believe we can expect it to remain to be existing because it is used widely in many helm charts.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the good catch 👍 Let me also add that both sprig's and helm's implementations of toJson seems to swallow errors, which sounds problematic to me. I'm now convinced to have our own implementation!
| data, err := json.Marshal(v) | ||
| return string(data), err | ||
| }, | ||
| "execTemplate": func(name string, ctx interface{}) (string, error) { |
There was a problem hiding this comment.
Could we use the template func bundled in text/template instead?
There was a problem hiding this comment.
that is a workaround for template not being a real function :) You can not pipe or capture its result
No I didn't verify it. Your question made me check for UserData calls and I found that I forgot to update normal ASG worker userdata calls. It should be in sync now and givent that template they are calling is identical it should work, but I'd like to ask for e2e test or apply it to a test cluster if you have it running locally. |
|
@redbaron Thanks for the fix. Sure, I'll kick the E2E script before I go to sleep 🛌 |
|
E2E passed. Thanks for your contribution as always, @redbaron 👍 |
* kubernetes-incubator/master: Update heapster k8s manifests accordingly to the upstream Add cluster kube-aws version to outputs Refactor code to reside in WithDefaultsFrom() Update go used to build the E2E container image to 1.8.3 to fix a build error Update Kubernetes to v1.7.0 Closes kubernetes-retired#744 Update CA to 0.6.0 Closes kubernetes-retired#725 Install Tiller by default Closes kubernetes-retired#656 Update golang to v1.8.3 Closes kubernetes-retired#720 Update the default etcd version to 3.2.1 Closes kubernetes-retired#726 Update OWNERS Inject stack name into userdata for nodepool workers (kubernetes-retired#735) Add CA a CriticalAddonsOnly toleration Ref kubernetes-retired#724 Update comment in cluster.yaml Drain node on spot instance termination notice as well Additional propagation of etcd version for etcdadm#722) Bugfix: CloudWatchLogging always disabled for Worker nodes Update ROADMAP.md
Fixes #727