ref(*): extract set, source, strategy and place into valuesource #219
Conversation
valuesource/strategy.go
Outdated
| // | ||
| // This matches the credentials required by the bundle to the credentials present | ||
| // in the Set, and then expands them per the definition in the Bundle. | ||
| func (s Set) ExpandCredentials(b *bundle.Bundle, stateless bool) (env, files map[string]string, err error) { |
There was a problem hiding this comment.
Looking more closely at how this function is used, I think I would move it off of valuesource/credential.Set and into a function in the action package. Doesn't have to happen in this PR (I can do that as a follow-up to mine) but really that is the only place it is used, is preparing for opForClaim and it doesn't quite fit into what this struct is responsible for I think. What do you think?
There was a problem hiding this comment.
Good point. I think it makes sense to make this change in this PR.
There was a problem hiding this comment.
I moved the method into action.go and updated its signature (no longer a method on a Set struct; takes a valuesource.Set object as a param). WDYT?
|
Sorry just realized this is still in draft. Are there more changes to be made? |
... so that it can be used for other sets needing source value resolution such as parameter sets. Signed-off-by: Vaughn Dice <vadice@microsoft.com>
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
|
Rebased off the latest changes. Note I also added the following commit: 581ddfb This updates the invalid source verbiage to be more general, as the source value may not be for a credential (e.g. it could be for a parameter, as can be the case in Porter). |
action/action.go
Outdated
| func opFromClaim(stateless bool, c claim.Claim, ii bundle.InvocationImage, creds credentials.Set) (*driver.Operation, error) { | ||
| env, files, err := creds.Expand(c.Bundle, stateless) | ||
| func opFromClaim(stateless bool, c claim.Claim, ii bundle.InvocationImage, creds valuesource.Set) (*driver.Operation, error) { | ||
| env, files, err := expandCredentials(&c.Bundle, creds, stateless) |
There was a problem hiding this comment.
As a suggested simplification, now that claim.Bundle is a value, we can change the signature of expandCredentials to take a value too and not need to change it to a pointer here and elsewhere (like in the tests) anymore.
|
I've been using this code in a test branch of Porter for a couple weeks now and it's been working well 👍 |
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
action/action_test.go
Outdated
| @@ -1051,3 +1050,84 @@ func TestSaveAction(t *testing.T) { | |||
| assert.Error(t, err, "the output should NOT be persisted") | |||
| }) | |||
| } | |||
|
|
|||
| func Test_expandCredentials(t *testing.T) { | |||
There was a problem hiding this comment.
nit: is there a reason for the different naming of this test?
(i.e. why not TestExpandCredentials?)
There was a problem hiding this comment.
No particular reason for the test name so updated to TestExpandCredentials 👍
| } | ||
|
|
||
| func (s Source) MarshalYAML() (interface{}, error) { | ||
| // TODO: use https://github.com/ghodss/yaml so that we don't need json and yaml defined |
There was a problem hiding this comment.
Is there an open issue for this?
There was a problem hiding this comment.
Thanks; created #222 to track this item (not intro'd in this PR).
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
Set,SourceandStrategystructs so that they can be used for other sets needing source value resolution, such as parameter sets.Ref getporter/porter#1068