Revise router template functions #5068

pecameron · 2017-08-24T15:10:57Z

OCP 3.7

PR 12882 refactored the router template functions.
This PR documents the changes.

pecameron · 2017-08-24T15:11:58Z

@louyihua @rajatchopra @knobunc PTAL

rajatchopra

Impeccable.
/lgtm

louyihua · 2017-08-25T01:16:54Z

install_config/router/customized_haproxy_router.adoc

-argument if the variable cannot be read or is empty.
+|`*processEndpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit, action string) []Endpoint*` |
+Returns the list of valid endpoints. When action is "shuffle", the order of endpoints is randomized.
+|`*env(variable, default ...string) string*` | Tries to get the named environment variable from the pod.


Here should be *env(variable string, defaults ...string) string*

louyihua · 2017-08-25T01:17:53Z

install_config/router/customized_haproxy_router.adoc

-subdomain for hosts (and paths) with a wildcard policy.
-|`*generateRouteRegexp(hostname, path string, wildcard bool) string*` | Generates a regular expression matching the route
-hosts (and paths). The first argument is the host name, the second is the path,
+|`*firstMatch(s string, allowedValues ...string) bool*` | Compares a given string to a list of allowed


The first argument of firstMatch is a regular expression.

pecameron · 2017-08-30T19:39:32Z

@louyihua @knobunc PTAL

knobunc

LGTM

louyihua · 2017-08-31T00:48:54Z

LGTM

Miciah · 2017-10-02T15:17:17Z

install_config/router/customized_haproxy_router.adoc

@@ -115,21 +115,25 @@ The *define* action names the file that will contain the processed template.
 [cols="2*", options="header"]
 |===
 |Function | Meaning
-|`*endpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit) []Endpoint*` | Returns the list of valid endpoints.


Is endpointsForAlias considered deprecated now?

I don't know. It is not currently used.

Miciah · 2017-10-02T15:19:26Z

install_config/router/customized_haproxy_router.adoc

+|`*processEndpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit, action string) []Endpoint*` |
+Returns the list of valid endpoints. When action is "shuffle", the order of endpoints is randomized.
+|`*env(variable string, default ...string) string*` | Tries to get the named environment variable from the pod.
+If it is not defined or empty, it returns the optional second argument.


This doesn't describe the behaviour when no second argument is given, or when more than two arguments are given.

Changed. Otherwise it returns the empty string. Other arguments are ignored.

OCP 3.7 PR 12882 refactored the router template functions. This PR documents the changes.

pecameron · 2017-10-03T13:39:45Z

@Miciah I addressed your questions and updated the PR. endpointsForAlias is in the code and there are no comments about it being deprecated. It is not currently used in the haproxy template. There is no telling if any customers are using it.

Miciah · 2017-10-03T15:28:00Z

@pecameron, thanks! Did you push the new changes? I don't see them yet.

Regarding endpointsForAlias, anybody who is using a custom template who has not rebased recently is using the old function. In order not to break custom templates, we should either document that the old function is deprecated (so that people with custom templates know to change to the new function) or commit to retaining the old function indefinitely (in which case we should document that).

This is probably not the best place to discuss it though; is the matter being tracked anywhere?

pecameron · 2017-10-06T13:14:40Z

@Miciah PTAL

Miciah · 2017-10-06T14:02:49Z

install_config/router/customized_haproxy_router.adoc

+|`*processEndpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit, action string) []Endpoint*` |
+Returns the list of valid endpoints. When action is "shuffle", the order of endpoints is randomized.
+|`*env(variable, default ...string) string*` | Tries to get the named environment variable from the pod.
+If it is not defined or empty, it returns the optional second argument. Otherwise, it returns an empty string.


What do you think of this? "If the value is empty or not defined and one or more optional arguments are provided, env returns the first non-empty argument. Otherwise, it returns an empty string."

@Miciah In practice there are 0 or 1 optional values. The environment variable either has a default or it doesn't.

@pecameron, ah, if we want to avoid documenting (and committing to) allowing more than 2 parameters, that's fine. Never mind!

vikram-redhat · 2018-07-18T23:25:03Z

@bfallonf - PTAL - this is quite old so review accordingly.

bfallonf · 2018-08-03T04:33:32Z

Sorry @pecameron , looks like this one escaped us. I'll merge this and cherry-pick to the right branches, and make a quick followup PR if I need to. Thanks!

bfallonf · 2018-08-03T04:34:12Z

/cherrypick enterprise-3.11

openshift-cherrypick-robot · 2018-08-03T04:34:43Z

@bfallonf: new pull request created: #11350

In response to this:

/cherrypick enterprise-3.11

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.

bfallonf · 2018-08-03T04:35:30Z

/cherrypick enterprise-3.10

openshift-cherrypick-robot · 2018-08-03T04:35:41Z

@bfallonf: new pull request created: #11351

In response to this:

/cherrypick enterprise-3.10

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.

bfallonf · 2018-08-03T04:35:48Z

/cherrypick enterprise-3.9

openshift-cherrypick-robot · 2018-08-03T04:35:58Z

@bfallonf: new pull request created: #11352

In response to this:

/cherrypick enterprise-3.9

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.

bfallonf · 2018-08-03T04:38:14Z

/cherrypick enterprise-3.7

openshift-cherrypick-robot · 2018-08-03T04:38:20Z

@bfallonf: new pull request created: #11353

In response to this:

/cherrypick enterprise-3.7

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.

pecameron mentioned this pull request Aug 24, 2017

Router template optimization openshift/origin#12882

Merged

rajatchopra approved these changes Aug 24, 2017

View reviewed changes

louyihua reviewed Aug 25, 2017

View reviewed changes

pecameron force-pushed the originPR12882 branch from 23bc318 to c0fd0bb Compare August 30, 2017 19:38

knobunc approved these changes Aug 30, 2017

View reviewed changes

Miciah reviewed Oct 2, 2017

View reviewed changes

Revise router template functions

c4be282

OCP 3.7 PR 12882 refactored the router template functions. This PR documents the changes.

pecameron force-pushed the originPR12882 branch from c0fd0bb to c4be282 Compare October 5, 2017 17:52

Miciah reviewed Oct 6, 2017

View reviewed changes

bfallonf merged commit 8e61251 into openshift:master Aug 3, 2018

bfallonf added branch/enterprise-3.7 branch/enterprise-3.9 branch/enterprise-3.10 branch/enterprise-3.11 labels Aug 3, 2018

bfallonf added this to the Next Release milestone Aug 3, 2018

openshift-cherrypick-robot mentioned this pull request Aug 3, 2018

[enterprise-3.11] Revise router template functions #11350

Merged

openshift-cherrypick-robot mentioned this pull request Aug 3, 2018

[enterprise-3.10] Revise router template functions #11351

Merged

openshift-cherrypick-robot mentioned this pull request Aug 3, 2018

[enterprise-3.9] Revise router template functions #11352

Merged

openshift-cherrypick-robot mentioned this pull request Aug 3, 2018

[enterprise-3.7] Revise router template functions #11353

Merged

Revise router template functions #5068

Revise router template functions #5068

Uh oh!

Conversation

pecameron commented Aug 24, 2017

Uh oh!

pecameron commented Aug 24, 2017

Uh oh!

rajatchopra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pecameron commented Aug 30, 2017

Uh oh!

knobunc left a comment

Choose a reason for hiding this comment

Uh oh!

louyihua commented Aug 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pecameron commented Oct 3, 2017

Uh oh!

Miciah commented Oct 3, 2017

Uh oh!

pecameron commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vikram-redhat commented Jul 18, 2018

Uh oh!

bfallonf commented Aug 3, 2018

Uh oh!

bfallonf commented Aug 3, 2018

Uh oh!

openshift-cherrypick-robot commented Aug 3, 2018

Uh oh!

bfallonf commented Aug 3, 2018

Uh oh!

openshift-cherrypick-robot commented Aug 3, 2018

Uh oh!

bfallonf commented Aug 3, 2018

Uh oh!

openshift-cherrypick-robot commented Aug 3, 2018

Uh oh!

bfallonf commented Aug 3, 2018

Uh oh!

openshift-cherrypick-robot commented Aug 3, 2018

Uh oh!

Uh oh!