-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 sprig functions to ClusterClass templates #6131
🌱 Add sprig functions to ClusterClass templates #6131
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, very nice small change :)
I think you got all places. Essentially we have two "template" variables and for each of those we have a validation and a place where it's used during patching.
docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with the go.mod update.
I think we should open an issue for more comprehensive patching documentation as this gets more complex. A few examples of valid patches and the functions we're offering here (doesn't have to be exhaustive.) I'll take that as an issue if you'd like.
a56f24e
to
98523a8
Compare
Some examples would be nice, but I would wait for providers to start using this so we can provide real use cases. |
Thx! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md
Outdated
Show resolved
Hide resolved
@@ -587,6 +587,9 @@ constant default value which can be specified in the schema is not enough. | |||
# If .vnetName is set, it is used. Otherwise, we will use `{{.builtin.cluster.name}}-vnet`. | |||
template: "{{ if .vnetName }}{{.vnetName}}{{else}}{{.builtin.cluster.name}}-vnet{{end}}" | |||
``` | |||
When writing templates, a subset of functions from [sprig](http://masterminds.github.io/sprig/) can be used to | |||
write expressing like e.g. `{{ .name | upper }}`. Only functions that are guaranteed to evaluate to the same result | |||
for a given input are allowed (e.g. `upper` or `max` can be used, while `now` or `randAlpha` can not be used). | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document all the available functions in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead refer to e.g. that documentation page: http://masterminds.github.io/sprig/
I would really prefer not duplicating parts of their docs (even if it's only a list of funcs) as it provides a lot of stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are over 70 template functions in sprig, I don't think it is doable to document here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking or importing works, from a user experience having them in one place (even if generate) is much more useful when implementing a clusterclass
docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md
Outdated
Show resolved
Hide resolved
98523a8
to
7d209df
Compare
7d209df
to
679df25
Compare
679df25
to
d6f7534
Compare
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
As per #6091 (comment) |
@fabriziopandini: #6131 failed to apply on top of branch "release-1.1":
In response to this:
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. |
Manual cherry-pick: #6191 |
What this PR does / why we need it:
This PR adds sprig functions to ClusterClass templates
Which issue(s) this PR fixes:
Fixes #6091