Add functionality to SourceMapList #2734
Conversation
Add methods to SourceMapList (the lists of parameters or credentials in a set) for working with the collection using item keys and converting to a map of resolved values Signed-off-by: Carolyn Van Slyck <[email protected]>
Signed-off-by: Carolyn Van Slyck <[email protected]>
pkg/secrets/strategy.go
Outdated
| // GetTypedResolvedValues converts the items to a map of key/value pairs, where the value is the type defined by the bundle. | ||
| // Validation errors are accumulated, and parameters are always passed back along with a validation error. | ||
| // If you care about valid parameters, check the error, but otherwise you can use this for display purposes and not have it blow up with no results. | ||
| func (l SourceMapList) GetTypedResolvedValues(bun cnab.ExtendedBundle) (map[string]interface{}, error) { |
There was a problem hiding this comment.
I copied this from elsewhere in porter while refactoring and just realized that the functionality was only tested higher up in the code, but never as a unit test for the function itself. Adding one now...
There was a problem hiding this comment.
Oops, never mind this needs to be defined directly on ParameterSet, and not on the generic SourceMapList. I'll remove it from this PR.
Signed-off-by: Carolyn Van Slyck <[email protected]>
GetTypedResolvedValues needs to lookup definitions by parameter name, which is too specific for where I've defined the function. I will move it to ParameterSet in another PR. Signed-off-by: Carolyn Van Slyck <[email protected]>
|
|
||
| // GetResolvedValue returns the resolved value of the specified name and a bool indicating if it was found. | ||
| // You must resolve the value before calling, it does to do resolution for you. | ||
| func (l SourceMapList) GetResolvedValue(name string) (interface{}, bool) { |
There was a problem hiding this comment.
could you use a type param like T here and then return a T? I'm not sure how deep you have to go into SourceMap to make that work, so maybe it's not worth it at the moment, not sure.
There was a problem hiding this comment.
yeah right now there's just one place in the code that uses this function, and casting the interface{} to a concrete type is easier than trying to get generics to work in this case.
But I'll keep it in mind if we start using it more. For the most part in Porter, we ignore the actual values of any parameters and just pass them around as interface{} to the bundle (the exception is the porter specific parameter that's inserted into every bundle built by porter, "porter-debug").
| @@ -130,18 +116,84 @@ func (s Source) MarshalYAML() (interface{}, error) { | |||
| return s.MarshalRaw(), nil | |||
| } | |||
|
|
|||
| type StrategyList []SourceMap | |||
| // SourceMapList is a list of mappings that can be access via index or the item name. | |||
| type SourceMapList []SourceMap | |||
There was a problem hiding this comment.
Should there be a name safe "Add" method?
There was a problem hiding this comment.
Right now it wouldn't be used, I don't have anything that mutates sets like that. Normally these are populated by unmarshaling a parameter/credential set.
I can add one though in case it comes in handy later, maybe for our tests.
There was a problem hiding this comment.
Actually I'm not sure what the universal behavior of that should even be? Refusing to append if it's already in the list? Overwrite it? I'd rather leave that up to the caller, especially since nothing in code would use it now. We can add something later if there's a common pattern that we want people to use.
There was a problem hiding this comment.
This struct is acting like an ordered set, do I have that right? If so, do the GetByName/HasName functions need to be faster than linear time? If so, maybe it'd be worth abstracting the list away and adding a function similar to:
Add(key, value, overwrite)
... that always writes the key/value pair if the key didn't already exist, and only overwrites a key/value pair if the key already existed and overwrite was true.
Then if you later want/need to make those Get/Has functions faster, you can, and callers are still free to decide the behavior they want? Not sure if this is too much overkill.
There was a problem hiding this comment.
The GetByName functionality is just for the tests and is only used once in production code (to read the porter-debug parameter).
The data structure is a map (unique names) represented as a list because it works better UX wise that way when the user is defining the data in Porter (and we can't change that at this point due to compatibility). VS code doesn't do a great job of autocomplete and suggesting what to type when using maps, vs arrays. With maps, when I try to autocomplete, since the key name could be anything, there isn't any autocomplete help. With arrays, I can ask for autocomplete and it creates an entry with all required fields for me, and helps people make bundles faster with less trips to the docs. Happy to show you sometime, and see if there's ways to improve.
Once the data is unmarshaled from porter.yaml into our data structure, we mostly work with it as a single set of data and don't retrieve items by name. However, when importing, detecting duplicate names would be nice.
I think a custom yaml marshal/unmarshal that lets SourceMapList BE a map in Go would get rid of the awkwardness. Let me play around with that idea a bit and see how disruptive that would be. Long term sticking with well understood data structures for this would make it easier to reason about so thank you for pointing out that I was making an odd one!
There was a problem hiding this comment.
This feels either like a rabbit hole or a really great idea that we can make generic and use for other parts of porter.yaml that are represented as a list but in code as a map.'
I'll give it one more day to see if it's worthwhile or not. 😂
Disambiguate GetResolvedValues and GetResolvedValue by renaming the converter function GetResolvedValues to ToResolvedValues, making it more clear that it converts the current struct into a new representation and doesn't just fetch data. Signed-off-by: Carolyn Van Slyck <[email protected]>
|
Closing in favor of #2750 |
What does this change
Add methods to SourceMapList (the lists of parameters or credentials in a set) for working with the collection using item keys and converting to a map of resolved values
What issue does it fix
Prep work for #2698
Notes for the reviewer
N/A
Checklist
Reviewer Checklist