-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ref(*): extract set, source, strategy and place into valuesource #219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think it makes sense to make this change in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call; updated!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is there a reason for the different naming of this test?
(i.e. why not TestExpandCredentials
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an open issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; created #222 to track this item (not intro'd in this PR).
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
Set
,Source
andStrategy
structs so that they can be used for other sets needing source value resolution, such as parameter sets.Ref getporter/porter#1068