Helper Functions for custom annotations #691
Conversation
| ) | ||
|
|
||
| // SplitInput splits the input from "," and returns an array of strings | ||
| func SplitInput(s string, delim string) []string { |
There was a problem hiding this comment.
there is no need for these functions to be public - they are not used in other packages
| return strings.TrimSpace(s) | ||
| } | ||
|
|
||
| // HelperFunctions to parse the annotations |
There was a problem hiding this comment.
I suggest removing this comment
docs/custom-annotations.md
Outdated
| @@ -86,6 +86,37 @@ If you'd like to use custom annotations with Mergeable Ingress resources, please | |||
|
|
|||
| * Minions do not inherent custom annotations of the master. | |||
|
|
|||
| ### Helper Functions | |||
|
|
|||
| Helper functions can be used in custom templates to transform the value of custom annotations. | |||
There was a problem hiding this comment.
| Helper functions can be used in custom templates to transform the value of custom annotations. | |
| Helper functions can be used in the Ingress template to parse the values of custom annotations. |
docs/custom-annotations.md
Outdated
|
|
||
| Helper functions can be used in custom templates to transform the value of custom annotations. | ||
|
|
||
| | Function | Description | |
There was a problem hiding this comment.
I suggest adding two additional columns to better document the functions.
| | Function | Description | | |
| | Function | Input Arguments| Return Arguments | Description | |
Additionally, see the following suggestions:
| splitinput | s, sep string | []string | Splits the string s into a slice of strings separated by the sep. | triminput | s string | string | Trims the trailing and leading whitespace from the string s. |
docs/custom-annotations.md
Outdated
| | `splitinput` | Splits the arguments at specified delimiter and returns an array of strings | | ||
| | `triminput` | Trims the trailing and leading whitespaces from the arguments | | ||
|
|
||
| For example, the following custom annotation custom.nginx.org/allowed-ips represents a comma separated list of IP addresses as a string. |
There was a problem hiding this comment.
| For example, the following custom annotation custom.nginx.org/allowed-ips represents a comma separated list of IP addresses as a string. | |
| Consider the following custom annotation `custom.nginx.org/allowed-ips`, which expects a comma-separated list of IP addresses: |
| } | ||
|
|
||
| func TestTrimHelperFunctions(t *testing.T) { | ||
| const tpl = `{{triminput (index .)}}` |
There was a problem hiding this comment.
| const tpl = `{{triminput (index .)}}` | |
| const tpl = `{{triminput .}}` |
| tmpl, err := template.New("testTemplate").Funcs(helperFunctions).Parse(tpl) | ||
| if err != nil { | ||
| t.Fatalf("Failed to parse template: %v", err) | ||
| } |
There was a problem hiding this comment.
consider adding a blank line here to improve readability. Same applies for TestTrimHelperFunctions
| slice := "foo,bar" | ||
| expected := "foo bar " | ||
| err = tmpl.Execute(&buf, slice) | ||
| t.Log(buf.String()) |
There was a problem hiding this comment.
do we need to print the buffer? Same applies for TestTrimHelperFunctions
There was a problem hiding this comment.
The other tests are following same pattern to print buf if it fails. So did same here. Let me know if it shoud be removed
There was a problem hiding this comment.
Consider removing it here.
The other tests do not check the generated config against the expected output -- they only check that there is no error returned. That's mostly why t.Log(buf.String()), so that the output could be checked visually. In contrast, this test checks the output against the expected output, so no printing is necessary.
| err = tmpl.Execute(&buf, slice) | ||
| t.Log(buf.String()) | ||
| if err != nil { | ||
| t.Fatalf("Failed to write template %v", err) |
There was a problem hiding this comment.
failed to execute the template?
| } | ||
| var buf bytes.Buffer | ||
|
|
||
| slice := "foo,bar" |
There was a problem hiding this comment.
I think input would suit better for the name of this variable. Same applies for TestTrimHelperFunctions
| slice := "foo,bar" | ||
| expected := "foo bar " | ||
| err = tmpl.Execute(&buf, slice) | ||
| t.Log(buf.String()) |
There was a problem hiding this comment.
Consider removing it here.
The other tests do not check the generated config against the expected output -- they only check that there is no error returned. That's mostly why t.Log(buf.String()), so that the output could be checked visually. In contrast, this test checks the output against the expected output, so no printing is necessary.
docs/custom-annotations.md
Outdated
| | `triminput` | Trims the trailing and leading whitespaces from the arguments | | ||
| | Function | Input Arguments| Return Arguments | Description | | ||
| | -------- | -------------- | ---------------- | ----------- | | ||
| | `split` | s, sep string | []string | Splits the string s into a slice of strings separated by the sep. | |
There was a problem hiding this comment.
| | `split` | s, sep string | []string | Splits the string s into a slice of strings separated by the sep. | | |
| | `split` | `s, sep string` | `[]string` | Splits the string `s` into a slice of strings separated by the `sep`. | |
docs/custom-annotations.md
Outdated
| | Function | Input Arguments| Return Arguments | Description | | ||
| | -------- | -------------- | ---------------- | ----------- | | ||
| | `split` | s, sep string | []string | Splits the string s into a slice of strings separated by the sep. | | ||
| | `trim` | s string | string | Trims the trailing and leading whitespace from the string s. | |
There was a problem hiding this comment.
| | `trim` | s string | string | Trims the trailing and leading whitespace from the string s. | | |
| | `trim` | `s string` | `string` | Trims the trailing and leading whitespace from the string `s`. | |
| // SplitInput splits the input from "," and returns an array of strings | ||
| func SplitInput(s string, delim string) []string { | ||
| // splitinput splits the input from "," and returns an array of strings | ||
| func splitinput(s string, delim string) []string { |
There was a problem hiding this comment.
For multiword names, the convention is to use mixed caps. Like "splitInput". See https://golang.org/doc/effective_go.html#mixed-caps
At the same time, as mentioned here #691 (comment), I suggest renaming to split. This will make the name simpler, as the suffix input seems unnecessary.
Same applies for triminput.
| var helperFunctions = template.FuncMap{ | ||
| "splitinput": SplitInput, //returns array of strings | ||
| "triminput": TrimInput, //returns string with trimmed leading and trailing spaces | ||
| "split": splitinput, //returns array of strings |
There was a problem hiding this comment.
I suggest removing these comments completely. The comment for split s already outdated. Having the docs in the doc is enough.
| // SplitInput splits the input from "," and returns an array of strings | ||
| func SplitInput(s string, delim string) []string { | ||
| // splitinput splits the input from "," and returns an array of strings | ||
| func splitinput(s string, delim string) []string { |
There was a problem hiding this comment.
The documentation for the function is not correct. I suggest removing it complety - the function is private and also strait-forward.
Same applies for triminput.
Proposed changes
Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue here in this description (not in the title of the PR).
Checklist
Before creating a PR, run through this checklist and mark each as complete.