support non-string template parameter substitution #11421

bparees · 2016-10-18T18:23:10Z

fixes #3946
fixes bug 1383812

bparees · 2016-10-18T18:33:22Z

@smarterclayton ptal.

bparees · 2016-10-18T18:33:29Z

@csrwng ptal also

bparees · 2016-10-18T18:33:49Z

@openshift/api-review needs review.

csrwng

Really minor nits, otherwise LGTM

csrwng · 2016-10-18T18:39:50Z

pkg/template/template.go

@@ -126,15 +130,32 @@ func GetParameterByName(t *api.Template, name string) *api.Parameter {

 // EvaluateParameterSubstitution replaces escaped parameters in a string with values from the
 // provided map.
-func (p *Processor) EvaluateParameterSubstitution(params map[string]api.Parameter, in string) string {
-	for _, match := range parameterExp.FindAllStringSubmatch(in, -1) {
+func (p *Processor) EvaluateParameterSubstitution(params map[string]api.Parameter, in string) (string, bool) {


You should mention in the comment what the bool is for

csrwng · 2016-10-18T18:40:49Z

pkg/template/template.go

+		if len(match) > 1 {
+			if paramValue, found := params[match[1]]; found {
+				in = strings.Replace(in, match[0], paramValue.Value, 1)
+				return in, false


really minor nit ... you should name your result variable differently (assign result := in at the beginning of the function). Seems strange returning in.

bparees · 2016-10-18T19:10:46Z

nits addressed

deads2k · 2016-10-20T15:10:52Z

pkg/template/template.go

+// match ${KEY}, KEY will be grouped
+var stringParameterExp = regexp.MustCompile(`\$\{([a-zA-Z0-9\_]+?)\}`)
+
+// match ${{KEY}} exact match only, KEY will be grouped


This is neat. I like it.

you can thank @smarterclayton. I wanted to do much crazier things w/ substitution :)

smarterclayton · 2016-10-20T22:13:25Z

pkg/util/stringreplace/object.go

+		} else {
+			b = []byte(fmt.Sprintf("{\"k\":%s}", s))
+		}
+		var data map[string]interface{}


var data interface{}

You shouldn't need the "as string" syntax.

without "asString" how does the encoder differentiate between "true" (the string) and true (the boolean value)?

and i don't understand the var data interface{} suggestion. i definitely don't know the json mechanisms as well as you, but i thought i needed to decode into a map[string]interface in order to get the library to decode the arbitrary json string into a proper go type. If i just decode a string (not part of a json map), it will always get turned into a string, not a specific data type.

that is, json.Unmarshal("true",&data) is never going to give me the boolean value that i need in some cases (and in other cases i need it to give me the string type)

if i'm still wrong let's chat tomorrow.

smarterclayton · 2016-10-20T22:14:11Z

pkg/template/template_test.go

+						"key2": "$${VALUE}",
+						"s1_s1": "${STRING_1}_${STRING_1}",
+						"s1_s2": "${STRING_1}_${STRING_2}",
+						"i1": "${{INT_1}}"


Add more tests that verify that this syntax in combination with before and after stays a string.

Or return an error when you get a template with "${{INT_1}}bar" (didn't we say we would validate for that)?

we can't return an error for that because it was valid syntax before this change, so we need to continue to tolerate it w/o throwing an error.

but i will add a test to ensure we don't mutate it.

smarterclayton · 2016-10-20T22:15:54Z

pkg/template/template.go

+var stringParameterExp = regexp.MustCompile(`\$\{([a-zA-Z0-9\_]+?)\}`)
+
+// match ${{KEY}} exact match only, KEY will be grouped
+var nonStringParameterExp = regexp.MustCompile(`^\$\{\{([a-zA-Z0-9\_]+)\}\}$`)


Need to decide whether to error on invalid usage of this, and if we do then the regex needs to be looser.

As is "${{FOO }}" is ignored which is very weird (ignoring seems wrong, erroring seems probably right, unless we think we can't safely error because of backcompat)

i think we can't safely error because of backward compatibility. Someone could legitimately have "${{BLAH}}" in a template somewhere. (or "${{BLA H}}" or "B${{LAH}}" or any of the other permutations the regex currently ignores). (Arguably even introducing the ${{KEY}} syntax could break existing templates that for some reason contained a value ${{KEY}} reference in a string field, but presumably we have to accept that. So perhaps you want to make the argument that since we're already introducing a (slightly) breaking change, we can expand the number of scenarios under which people will be broken?)

That said, you'll ultimately get an error anyway if you were attempting parameter substitution into a non-string field and the substitution didn't happen because you didn't provide a valid parameter name, or because you included a prefix/suffix on the ${{KEY}}} parameter reference.

that is, you'll ultimately get an error because the resulting api object won't be valid (will contain a string value for an int/boolean/float field)

smarterclayton · 2016-10-20T22:18:23Z

Only remaining question is error and validation. Rest is fine.

bparees · 2016-10-21T04:56:56Z

@smarterclayton added requested additional tests but will need to discuss

whether to throw an error on invalid ${{KEY}} refs
your asString comment

smarterclayton · 2016-10-21T19:05:24Z

pkg/util/stringreplace/object.go

 	case reflect.String:
 		if !v.CanSet() {
-			glog.Infof("Unable to set String value '%v'", v)
-			return
+			return fmt.Errorf("Unable to set String value '%v'", v)


nit: remember all your errors need to be lowercase first char

We may actually want to be able to walk everything and get errors on specific fields. I.e. if you have 3 failures we want to report them all, not just the first one.

will change the error case.

as for multiple errors, that sort of thing hasn't been a concern users have raised so far w/ the existing template processing error behavior and i'd prefer to entertain that as a follow on at this point.

error case updated

smarterclayton · 2016-10-21T19:08:05Z

pkg/util/stringreplace/object.go

+				// which is an error when decoding in json(only "true", "false", and numeric
+				// values can be unquoted), so try wrapping the value in quotes so it will be
+				// properly converted to a string type during decoding.
+				b = []byte(fmt.Sprintf("\"%s\"", s))


Just call val = reflect.ValueOf(s) here. If it's a string, it's a string.

bparees · 2016-10-22T15:34:02Z

@smarterclayton ok i think this is ready for a final review.
[test]

smarterclayton

More tests

smarterclayton · 2016-10-22T18:48:18Z

pkg/template/template_test.go

+						"key2": "$${VALUE}",
+						"s1_s1": "${STRING_1}_${STRING_1}",
+						"s1_s2": "${STRING_1}_${STRING_2}",
+						"i1": "${{INT_1}}",


Create test conditions for the "error" case is described above. Also bad json substitution error case. I'd like a test that covers all the "broken" cases as well as the working cases.

Create test conditions for the "error" case is described above

not sure which case you're referring to, but there are three tests for "invalid" ${{KEY}} substitutions ("a${{KEY}}", "${{KEY}}a", and "${{INVALID_PARAMETER}}", all of which ensure that the value isn't touched in those cases (backwards compatible behavior).

Also bad json substitution error case

There is no "bad json substitution" scenario. if you use the ${{KEY}} syntax w/ a random string parameter value(non-json value), you end up with a quoted string because we fail to decode it and then use it as a direct string value instead. that's tested by the "quoted_string" key test. (ensures that if you use the ${{KEY}} syntax but the parameter value is not an int/bool, you end up with a quoted string containing the parameter value)

I don't think there's any case where we would explicitly error based on the substitution logic, we will either: do a string substitution instead of an int/boolean substitution, or we will do no substitution (leaving a string value with a ${{FOO}} reference embedded).

Those things might result in an invalid api object, but we've never done checking for that sort of thing, the template processing is api object agnostic.

Guessing there is still a scenario you're thinking of that i'm missing but i don't see it at the moment.

Added one more test for a "${{INVALID PARAMETER}}" reference (should be untouched)

bparees · 2016-10-23T15:22:33Z

flake #11094
[test]

deads2k · 2016-10-24T12:18:05Z

I think we all like the format. Marked API approved.

bparees · 2016-10-24T18:01:51Z

@smarterclayton final set of tests added. this does work w/ syntaxes like:

parameter reference:

        "selector": "${{SELECTOR}}",

json parameter value:

  "parameters": [
    {
      "name": "SELECTOR",
      "description": "administrator username",
      "value": "{\"name\": \"frontend\"}"
    },

results in:

 "selector": {
                    "name": "frontend"
                },

bparees · 2016-10-24T20:01:30Z

flake #11016
[test]

openshift-bot · 2016-10-24T21:30:30Z

Evaluated for origin test up to 23eb8fb

openshift-bot · 2016-10-24T22:56:40Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10582/) (Base Commit: f1d0dd2)

bparees · 2016-10-25T18:17:25Z

@pweil- or @smarterclayton are you signing off on this going in despite being past dcut?

smarterclayton · 2016-10-25T18:27:27Z

Lgtm once you go green

On Oct 24, 2016, at 5:29 PM, OpenShift Bot [email protected] wrote:

continuous-integration/openshift-jenkins/test SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10577/) (Base
Commit: f1d0dd2
f1d0dd2
)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11421 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_p6T-hKDVXrIfJvV_pyXQiDk-5wjiks5q3SMlgaJpZM4KaIIb
.

bparees · 2016-10-25T18:28:46Z

extended test passed despite github not reflecting that and travis is doing the govet shadow spurious failure but i've kicked it for a rerun. waiting for QE to agree to test it anyway before i merge.

smarterclayton · 2016-10-25T18:53:32Z

I'm looking for the test to actually demonstrate the complex json
substitution failure (convert to string) so someone who changes the
behavior later on explicitly has to do something about it.

On Oct 23, 2016, at 2:59 AM, Ben Parees [email protected] wrote:

@bparees commented on this pull request.

In pkg/template/template_test.go
#11421:

@@ -177,6 +177,59 @@ func TestParameterGenerators(t *testing.T) {
}
}

+func TestProcessValue(t *testing.T) {

var template api.Template
if err := runtime.DecodeInto(kapi.Codecs.UniversalDecoder(), []byte(`{

  "kind":"Template", "apiVersion":"v1",

```
  "objects": [
```
```
      {
```

          "kind": "Service", "apiVersion": "v${VALUE}",

```
          "metadata": {
```
```
              "labels": {
```
```
                  "key1": "${VALUE}",
```
```
                  "key2": "$${VALUE}",
```

                  "s1_s1": "${STRING_1}_${STRING_1}",

                  "s1_s2": "${STRING_1}_${STRING_2}",

```
                  "i1": "${{INT_1}}",
```

Create test conditions for the "error" case is described above

not sure which case you're referring to, but there are three tests for
"invalid" ${{KEY}} substitutions ("a${{KEY}}", "${{KEY}}a", and
"${{INVALID_PARAMETER}}", all of which ensure that the value isn't touched
in those cases (backwards compatible behavior).

Also bad json substitution error case

There is no "bad json substitution" scenario. if you use the ${{KEY}}
syntax w/ a random string parameter value(non-json value), you end up with
a quoted string because we fail to decode it and then use it as a direct
string value instead. that's tested by the "quoted_string" key test.
(ensures that if you use the ${{KEY}} syntax but the parameter value is not
an int/bool, you end up with a quoted string containing the parameter value)

I don't think there's any case where we would explicitly error based on the
substitution logic, we will either: do a string substitution instead of an
int/boolean substitution, or we will do no substitution (leaving a string
value with a ${{FOO}} reference embedded).

Those things might result in an invalid api object, but we've never done
checking for that sort of thing, the template processing is api object
agnostic.

Guessing there is still a scenario you're thinking of that i'm missing but
i don't see it at the moment.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11421, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p2SrMqH38YVR__TOBPqvwhPVfe4mks5q2wXPgaJpZM4KaIIb
.

bparees · 2016-10-26T03:37:37Z

[textextended][extended:core(openshift pipeline build Manual deploy)]

bparees · 2016-10-26T03:54:29Z

[merge]

openshift-bot · 2016-10-26T04:05:26Z

Evaluated for origin merge up to 23eb8fb

openshift-bot · 2016-10-26T04:13:30Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10657/) (Base Commit: ca101d7) (Image: devenv-rhel7_5245)

bparees force-pushed the int_templates branch 2 times, most recently from 94211e7 to 09a8a90 Compare October 18, 2016 18:33

bparees added the needs-api-review label Oct 18, 2016

bparees force-pushed the int_templates branch 3 times, most recently from 5654894 to 3ef73bd Compare October 18, 2016 18:36

csrwng reviewed Oct 18, 2016

View reviewed changes

bparees force-pushed the int_templates branch from 3ef73bd to 0a01a54 Compare October 18, 2016 19:10

deads2k reviewed Oct 20, 2016

View reviewed changes

smarterclayton reviewed Oct 20, 2016

View reviewed changes

bparees force-pushed the int_templates branch from 0a01a54 to 6b98e30 Compare October 21, 2016 04:56

bparees force-pushed the int_templates branch from 6b98e30 to b4be712 Compare October 21, 2016 18:50

smarterclayton reviewed Oct 21, 2016

View reviewed changes

bparees force-pushed the int_templates branch from b4be712 to d10943b Compare October 21, 2016 19:34

smarterclayton reviewed Oct 22, 2016

View reviewed changes

bparees force-pushed the int_templates branch from d10943b to 11317b8 Compare October 23, 2016 07:04

deads2k added api-approved and removed needs-api-review labels Oct 24, 2016

bparees force-pushed the int_templates branch from 11317b8 to b96243f Compare October 24, 2016 17:59

support non-string template parameter substitution

23eb8fb

bparees force-pushed the int_templates branch from b96243f to 23eb8fb Compare October 24, 2016 20:13

bparees mentioned this pull request Oct 24, 2016

document new non-string parameter syntax openshift/openshift-docs#3100

Merged

openshift-bot merged commit 20bcec1 into openshift:master Oct 26, 2016

bparees deleted the int_templates branch October 31, 2016 19:53

jorgemoralespou mentioned this pull request Jan 11, 2017

OpenShift template fails due to string provided instead of integer edseymour/ab-docker#1

Open

support non-string template parameter substitution #11421

support non-string template parameter substitution #11421

Conversation

bparees commented Oct 18, 2016 • edited Loading

bparees commented Oct 18, 2016

bparees commented Oct 18, 2016

bparees commented Oct 18, 2016

csrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Oct 20, 2016

bparees commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Oct 22, 2016

smarterclayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Oct 23, 2016

deads2k commented Oct 24, 2016

bparees commented Oct 24, 2016

bparees commented Oct 24, 2016

openshift-bot commented Oct 24, 2016

openshift-bot commented Oct 24, 2016

bparees commented Oct 25, 2016

smarterclayton commented Oct 25, 2016

bparees commented Oct 25, 2016

smarterclayton commented Oct 25, 2016

@bparees commented on this pull request.

bparees commented Oct 26, 2016

bparees commented Oct 26, 2016

openshift-bot commented Oct 26, 2016

openshift-bot commented Oct 26, 2016 • edited Loading

bparees commented Oct 18, 2016 •

edited

Loading

openshift-bot commented Oct 26, 2016 •

edited

Loading