fix(*): update parameter set logic to support file types #1137
Conversation
|
Oh! I just figured that the respective plugins (secret, or host) would take care of this. Gonna review later but this makes me wonder if we need to move the logic for file into a different place perhaps (like in a wrapper around host) so that it can't be accidentally bypassed? I haven't looked at your changes yet though, so don't feel the need to change anything based on my rando musings! |
pkg/porter/cnab.go
Outdated
| if len(o.ParameterSets) > 0 { | ||
| // Load the manifest as it may be needed to determine if any | ||
| // parameters are of the Porter-exclusive type of 'file' | ||
| err := p.LoadManifestFrom(o.File) |
There was a problem hiding this comment.
This results in the manifest being loaded twice because we are loading it here (sometimes) and also in p.applyDefaultOptions (always). I think this inconsistency may lead to bugs where we aren't sure when we can rely on the manifest being set, and by when.
There was a problem hiding this comment.
Good call. My strategy on consolidation is to move the p.applyDefaultOptions into the Validate method on sharedOptions, to be sure the manifest is loaded prior to parsing parameters. This also has the advantage of removing duplicated invocations from the action files (install.go, etc.) WDYT?
Added in the following commit: 0cf52ae
| if param.Name == paramDef.Name { | ||
| // Pass through value (filepath) directly to resolvedParameters | ||
| resolvedParameters[param.Name] = param.Source.Value | ||
| // Eliminate this param from pset to prevent its resolution by |
There was a problem hiding this comment.
What if instead of loading the manifest here and trying to check the parameters before it gets to the cnab-go code, we wrapped the cnab-go host secret store, and extended it in porter to support our custom file type property?
Do you think it would be possible to change our support for the file type from a "hack" (that we shim in wherever we can fit it) to instead something that we override and support in the same struct/place that cnab-go supports the other types?
There was a problem hiding this comment.
It may be possible, but my initial concern on going this direction was that this file type has classically been exclusive to Porter. As cnab-go is intended to be the Golang reference library for the CNAB Spec, which doesn't mention this type, I didn't think it was a proper fit. However, I agree: it would be much cleaner to move support/override-ability into this library.
Say we do intend to proceed with adding to the cnab-go library. As there is pre-existing code to clean up (not intro'd in this PR), I'd definitely like to work off of a new issue mentioning this broader scope. The question would then be: do we want this fix (or a variant thereof) in the near-term to ensure functionality until the refactor (and corresponding cnab-go PR(s)) are scheduled to be worked on?
There was a problem hiding this comment.
Oops, miscommunication here. I was suggesting that we wrap the type defined in cnab-go responsible for resolving the parameter set and then extend it "inside of porter to support the file type". So the file type would continue to be porter only.
There was a problem hiding this comment.
Ah, thank you. I think I have a better understanding now. I'll work with you to outline a strategy in a follow-up refactor in this area.
Signed-off-by: Vaughn Dice <[email protected]>
Signed-off-by: Vaughn Dice <[email protected]>
vdice
force-pushed
the
fix/param-set-with-file-param
branch
from
0ad0b0e to
0cf52ae
Compare
| @@ -93,6 +93,11 @@ func (o *sharedOptions) Validate(args []string, p *Porter) error { | |||
| return err | |||
| } | |||
|
|
|||
| err = p.applyDefaultOptions(o) | |||
What does this change
filein the Porter manifest via Parameter SetsWhat issue does it fix
Closes #1130
Notes for the reviewer
📁
Checklist
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇♀️