Enhances how we're handling sourcing #162
Conversation
ghost
assigned 
…put sourcing This PR updates how we handle slice elements for doing parameter/credential/ouput sourcing. We probably should think about revisitng the entire process for this in the future. In summary, we are using reflectwalk to handle walking the manifest. This involves handling a couple call backs, notably "SliceElement" and "MapElement". The Slice Element assumed that it would see something of the form source: <x.y.z> as a string. In reality, if that's not wrapped in quote, Go and YAML see that as a Map, but the map callback doesn't get called to handle that. This was resulting in the block not getting rewritten and the exec mixin was exploding (probably any would have). This PR updates the logic of the slice elem processing to check and see if it's a map and handle it in a similar manner to a top level map entry in the yaml. Closes: getporter#158
jeremyrickard
force-pushed
the
fix-sourcing
branch
from
f9cdd1f
to
0597f8e
Compare
| // command: bash | ||
| // arguments: | ||
| // - -c | ||
| // - "source: bundle.parameters.command" |
There was a problem hiding this comment.
I don't want to make this harder, because I agree that we need to rethink how we do sourcing anyway. But when I look at the two here,
- "source: bundle.parameters.command"
- source: bundle.parameters.command
My reaction is that the first one shouldn't even be parsed by porter and should be ignored really. It's only the second that matches how source works everywhere else.
Does that simplify this PR? If so, can we nix the first case "source as a string"? If it makes it harder, then leave it as it. 😀
| if sk.Kind() == reflect.Interface { | ||
| sk = sk.Elem() | ||
| } | ||
| //if the key is a string, and the string is source, then we should try |
There was a problem hiding this comment.
Like if we stop supporting - "source: bundle...." does this entire block go away?
There was a problem hiding this comment.
No, the opposite. This got added to support the other case where there are no quotes.
This is the part that handles the quotes:
v, ok := val.Interface().(string)
if ok {
//if the array entry is a string that matches source:...., we should replace it
re := regexp.MustCompile("source:\\s?(.*)")
matches := re.FindStringSubmatch(v)
if len(matches) > 0 {
source := matches[1]
r, err := m.resolveValue(source)
if err != nil {
return errors.Wrap(err, "unable to source value")
}
val.Set(reflect.ValueOf(r))
}
}
There was a problem hiding this comment.
Oh noes, I've been found out. I have a really hard time reading this reflection code.
I'm ok merging this as-is. Up to you if you think we can just remove the "source: bundle...." case and make things simpler or not.
| command: bash | ||
| arguments: | ||
| - -c | ||
| - source: bundle.parameters.command |
There was a problem hiding this comment.
This looks natural to me, based on how we do source elsewhere.
Ok I've said it 3 ways. I'll stop now. 😇
There was a problem hiding this comment.
Yeah, to support this form, the change i added is needed. Both this and "source:...." are valid YAML, so I think we should support both for right now.
We should probably fix this in general though..but also:
arguments:
- asdadadad
- {bundle.params.blah}will probably have similar stupid YAML processing outcomes for example, that block above gets turned into:
args:
- blah
-
bundle.parameters.blah: ~by a validator, which is even worse. I think in that case it might require quotes to make things simpler.
There was a problem hiding this comment.
rawr 🦁 Ok I see why this is hard.
I say ship it and then reasses later.
| command: bash | ||
| arguments: | ||
| - -c | ||
| - source: bundle.parameters.command |
There was a problem hiding this comment.
rawr 🦁 Ok I see why this is hard.
I say ship it and then reasses later.
- added bundle variables (after porter [PR #162](getporter/porter#162) - full path to commands used
This PR updates how we handle slice elements for doing parameter/credential/ouput sourcing.
We probably should think about revisiting the entire process for this in the future.
In summary, we are using reflectwalk to handle walking the manifest. This involves handling a couple
call backs, notably "SliceElement" and "MapElement". The Slice Element assumed that it would see something
of the form source: <x.y.z> as a string. In reality, if that's not wrapped in quote, Go and YAML see that
as a Map, but the map callback doesn't get called to handle that. This was resulting in the block not getting
rewritten and the exec mixin was exploding (probably any would have). This PR updates the logic of the slice elem processing to check and see if it's a map and handle it in a similar manner to a top level map entry in the yaml.
Closes: #158