feat(*): add logic to resolve/interpolate output values in a Porter manifest #840
Conversation
|
I haven't reviewed this yet but one thing that is important to note about this change is that it creates bundles that can only be run by Porter because it relies upon the client injecting claim data into the runtime at a specific point What is our thoughts/policy for changes like this? Is it okay as a prototype that we then use to get an upstream spec change in? But we make sure to document it as experimental which may be removed once the spec is finalized? Or do we want to avoid doing this like this and always do spec first so that we don't make "porter only bundles" even for experimental stuff? We kind of have this as well with dependencies, though I think the other clients respect the spec around required custom extensions, and refuse to try to run those bundles that need dependency support. |
|
Good point @carolynvs . While mounting the claim to |
|
My vote is to follow/block on the spec and only make things that are porter only (like dependencies) when we are really prototyping something new and easy to warn people is experimental. This is so subtly different that no one would expect it to not work with other tools. |
|
@carolynvs now that cnabio/cnab-spec#300 is merged, this should be ready for review. |
There was a problem hiding this comment.
I'm having trouble telling if this preserved the existing functionality of how {{ bundle.outputs.NAME }} worked. Where a step within an action can produce an output and the next step within that action can consume it using bundle.outputs.NAME.
I was expecting to see resolveOutputs first look in the step outputs (bun.outputs) and then fallback to the output value from the claim. Did I miss this part in the code?
pkg/runtime/runtime-manifest.go
Outdated
| // e.g. via {{ bundle.outputs.foo }} | ||
| // It looks up the claim for the value written by the last action and, if exists, returns this value | ||
| func (m *RuntimeManifest) resolveOutput(output manifest.OutputDefinition) (string, error) { | ||
| if m.Action != manifest.ActionInstall { |
There was a problem hiding this comment.
I believe we have upcoming work to support running stateless custom actions before install is executes, i.e. without a claim. I suggest changing this to check if the claim is present, and if not, skip without relying on the action.
There was a problem hiding this comment.
Good call. Will update to check if the claim is present, as suggested.
Indeed, I'm adding this now. Working on integration test coverage for these two cases (step-level and bundle-level outputs). |
vdice
force-pushed
the
feat/resolve-outputs
branch
from
1614e99 to
8bc2b15
Compare
|
Ok @carolynvs , this is back and ready for re-review when convenient. Thanks! |
pkg/runtime/runtime-manifest.go
Outdated
| return m.outputs[output.Name], nil | ||
| } | ||
|
|
||
| if claimBytes, err := m.FileSystem.ReadFile(config.ClaimFilepath); err == nil { |
There was a problem hiding this comment.
nit: How about we make a new function loadClaim and then run it from Validate (which is called once before any steps are run). Then we don't reparse the claim for each output reference.
There was a problem hiding this comment.
Definitely, good thinking. This logic has been revised appropriately.
| m.sensitiveValues = append(m.sensitiveValues, output) | ||
| // Iterate through the runtime manifest's step outputs and mask by default | ||
| for _, stepOutput := range m.outputs { | ||
| // TODO: check if output declared sensitive or not; currently we default always sensitive |
There was a problem hiding this comment.
Do we need to create a follow-up issue to track this todo?
There was a problem hiding this comment.
Good call; I created #855 and added a link below this comment.
vdice
force-pushed
the
feat/resolve-outputs
branch
from
a9f795a to
209e760
Compare
…anifest Signed-off-by: Vaughn Dice <[email protected]>
Signed-off-by: Vaughn Dice <[email protected]>
Signed-off-by: Vaughn Dice <[email protected]>
Signed-off-by: Vaughn Dice <[email protected]>
Signed-off-by: Vaughn Dice <[email protected]>
Signed-off-by: Vaughn Dice <[email protected]>
vdice
force-pushed
the
feat/resolve-outputs
branch
from
209e760 to
d24d773
Compare
What does this change
Note:
Do not merge yet-Blocked by/Depends on cnabio/cnab-spec#300{{ bundle.outputs.foo }}can now be used in a step for actionnext, which will pullfoo's value from the claim, assuming the action that produced the current claim (say,install) produced an output offoo.One caveat here is that currently, until work is done to address #836, interpolation of these values will only be possible if the previous action supplied the output needed. Otherwise, outputs don't yet span more than one action if the actions spanned don't also produce the same output.
What issue does it fix
Closes #834
Notes for the reviewer
Checklist