Initial attempt to wire-up parameters #66

jeremyrickard · 2018-11-29T20:01:35Z

This uses mitchellh's reflectwalk to walk the step's Data map. It currently
handles replacing things of the form source: bundle.parameter.name in a map
or an Array. We can implement additional callbacks later on if needed to handle
other datatypes that might be in the YAML.

For example:

name: HELLO
version: 0.1.0
invocationImage: jeremyrickard/porter-hello:latest

parameters:
  - name: hello
    default: jeremy

install:
- description: "Say Hello World"
  exec:
    command: bash
    arguments:
    - -c
    - echo Hello World
    parameters:
      thing:
        source: bundle.parameters.hello
      something: source
      hello: 5
- description: "Say another"
  exec:
    command: echo
    arguments:
    - "source: bundle.parameters.hello"
    - "something else"

Both the thing entry in parameters and the first entry of the arguments array will be updated to the value of the "hello" parameter.

This uses mitchellh's reflectwalk to walk the step's Data map. It currently handles replacing things of the form source: bundle.parameter.name in a map or an Array. We can implement additional callbacks later on if needed to handle other datatypes that might be in the YAML.

jeremyrickard · 2018-11-29T20:03:50Z

pkg/porter/resolver.go

+	return nil
+}
+
+func (p *Porter) Slice(val reflect.Value) error {


This and the map function are empty just to implement the interface.

We have to implement this interface because we are passing in an instance of porter to reflectwalk.Walk, yeah?

You're closer to this than me, do you think it's a small enough change to have this pass in just the manifest instead?

jeremyrickard · 2018-11-29T20:04:43Z

pkg/porter/resolver.go

+}
+
+func (p *Porter) resolveValue(key string) (interface{}, error) {
+	source := strings.Split(key, ".")


the handling of this probably needs to change as we get more complicated "sourcing" like when dependencies are introduced.

jeremyrickard · 2018-11-29T20:07:08Z

pkg/porter/resolver.go

+		v = v.Elem()
+	}
+	if kind := v.Kind(); kind == reflect.Map {
+		if len(v.MapKeys()) == 1 {


if something has more than one key, it seems like not something to replace so this is short circuiting that logic

Makes sense, probably a good idea to put what you just said in a comment so that we don't forget what's going on here. 😀

This is really good to have because in another part of the manifest I want to use source again, and this lets me get away with it. e.g.

dependencies: - name: mysql connections: - source: bundle.parameters.foo destination: bundle.dependencies.mysql.parameters.foo

jeremyrickard · 2018-11-29T20:30:14Z

Right now, if I encounter a "source: <...>" thing that isn't resolvable, that's an error condition. After thinking about it a bit, it seems like we should maybe just log that that status occured but not error to cover situations where someone wanted to use that as a map key or it's in a string that happens to match that regex?

carolynvs

This is good to merge as-is if you want. Everything I have in here is just fine as follow-ups.

carolynvs · 2018-11-29T20:43:18Z

Gopkg.lock

@@ -290,6 +290,14 @@
  pruneopts = "NUT"
  revision = "95e2e42e3ece7421c14512f79f796fe3f5d7e5dc"

+[[projects]]
+  digest = "1:44e35ced240c6d8316dc8d604a73774ab052d40ebe636fd8f2ba5934f6329034"
+  name = "github.com/mitchellh/reflectwalk"


I love this package, so useful. 💕

carolynvs · 2018-11-29T20:47:47Z

pkg/porter/resolver.go

+		v = v.Elem()
+	}
+	if kind := v.Kind(); kind == reflect.Map {
+		if len(v.MapKeys()) == 1 {


Makes sense, probably a good idea to put what you just said in a comment so that we don't forget what's going on here. 😀

carolynvs · 2018-11-29T20:49:00Z

pkg/porter/resolver.go

+					value := kv.String()
+					replacement, err := p.resolveValue(value)
+					if err != nil {
+						return errors.Wrap(err, "unable to source value")


Do we have enough context here that the message could say which value it couldn't find? Or is that baked in the error that we are wrapping?

carolynvs · 2018-11-29T20:51:52Z

pkg/porter/resolver.go

+	return nil
+}
+
+func (p *Porter) Slice(val reflect.Value) error {


We have to implement this interface because we are passing in an instance of porter to reflectwalk.Walk, yeah?

You're closer to this than me, do you think it's a small enough change to have this pass in just the manifest instead?

carolynvs · 2018-11-29T20:53:18Z

pkg/porter/resolver.go

+		v = v.Elem()
+	}
+	if kind := v.Kind(); kind == reflect.Map {
+		if len(v.MapKeys()) == 1 {


This is really good to have because in another part of the manifest I want to use source again, and this lets me get away with it. e.g.

dependencies: - name: mysql connections: - source: bundle.parameters.foo destination: bundle.dependencies.mysql.parameters.foo

carolynvs · 2018-11-29T20:54:15Z

pkg/porter/resolver.go

+			source := matches[1]
+			r, err := p.resolveValue(source)
+			if err != nil {
+				return errors.Wrap(err, "unable to source value")


Same as above, can we make this error message provide a bit more context into exactly which source node isn't resolving?

carolynvs · 2018-11-29T20:56:16Z

pkg/porter/resolver.go

+				} else if param.Destination == nil && param.Destination.Path != "" {
+					replacement = param.Destination.Path
+				} else {
+					return nil, errors.New("unknown parameter definition")


Is the problem here that the parameter that it's trying to source from isn't declared? Maybe this error should be reference to declared parameter bundle.parameters.foo in <insert some context so we know which ones is borked.

carolynvs · 2018-11-29T20:56:41Z

pkg/porter/resolver.go

+				} else if cred.EnvironmentVariable != "" {
+					replacement = os.Getenv(cred.EnvironmentVariable)
+				} else {
+					return nil, errors.New("unknown credential definition")


same as above, if we can, let's provide more context

carolynvs · 2018-11-29T20:57:28Z

pkg/porter/resolver.go

+			}
+		}
+	} else {
+		return nil, errors.New(fmt.Sprintf("unknown parameter source: %s", source[1]))


What is this error case? I can't quite tell from the error message. Is it that the key used is invalid, e.g. not path or environment variable?

carolynvs · 2018-11-29T20:58:14Z

pkg/porter/resolver_test.go

+		},
+	}
+
+	os.Setenv("PERSON", "Ralpha")


:excellent:

carolynvs · 2018-11-29T21:16:24Z

After thinking about it a bit, it seems like we should maybe just log that that status occured but not error to cover situations where someone wanted to use that as a map key or it's in a string that happens to match that regex

Do you have an example of when we could get a false positive? I would hate to make it super easy for a typo in the source path to go unnoticed. Like I fat finger bundle.dependencies.icantspell and then don't see the warning in the scrollback, and then wonder why the heck nothing works days later when I try to use the bundle I built. 😀

If we do have cases where it doesn't resolve, and that's expected, maybe let people add a flag for strict resolution vs. loose? So that we fail immediately, then let them know that if we are being too aggressive in our matching, use a flag when building to ignore the problem?

jeremyrickard · 2018-11-29T21:17:29Z

After thinking about it a bit, it seems like we should maybe just log that that status occured but not error to cover situations where someone wanted to use that as a map key or it's in a string that happens to match that regex

Do you have an example of when we could get a false positive? I would hate to make it super easy for a typo in the source path to go unnoticed. Like I fat finger bundle.dependencies.icantspell and then don't see the warning in the scrollback, and then wonder why the heck nothing works days later when I try to use the bundle I built.

If we do have cases where it doesn't resolve, and that's expected, maybe let people add a flag for strict resolution vs. loose? So that we fail immediately, then let them know that if we are being too aggressive in our matching, use a flag when building to ignore the problem?

No, I was mostly thinking proactively, but I agree with that case beacuse I can't spell.

…wire-up-param

jeremyrickard commented Nov 29, 2018

View reviewed changes

Added a couple of comments

94a3279

carolynvs approved these changes Nov 29, 2018

View reviewed changes

jeremyrickard added 4 commits November 30, 2018 09:46

Updated to move resolve logic into config pkg and defined on manifest

b60170f

Merge branch 'master' into wire-up-param

02305ce

Fix unit-tests after refactor

1cc8457

Merge branch 'wire-up-param' of github.com:jeremyrickard/porter into …

5de1770

…wire-up-param

jeremyrickard merged commit 62c3742 into getporter:master Nov 30, 2018

carolynvs mentioned this pull request Dec 3, 2018

Wire parameters and credentials from the bundle into the mixin #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial attempt to wire-up parameters #66

Initial attempt to wire-up parameters #66

jeremyrickard commented Nov 29, 2018 •

edited

Loading

jeremyrickard Nov 29, 2018

carolynvs Nov 29, 2018

jeremyrickard Nov 29, 2018

jeremyrickard Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

jeremyrickard commented Nov 29, 2018

carolynvs left a comment

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs Nov 29, 2018

carolynvs commented Nov 29, 2018 •

edited

Loading

jeremyrickard commented Nov 29, 2018

Initial attempt to wire-up parameters #66

Initial attempt to wire-up parameters #66

Conversation

jeremyrickard commented Nov 29, 2018 • edited Loading

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

jeremyrickard commented Nov 29, 2018

carolynvs 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

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

carolynvs commented Nov 29, 2018 • edited Loading

jeremyrickard commented Nov 29, 2018

jeremyrickard commented Nov 29, 2018 •

edited

Loading

carolynvs commented Nov 29, 2018 •

edited

Loading