Open
Conversation
It's a common pattern in Porter to represent a map of items, with a unique key, as a list in the porter.yaml or a file representation of the resource because the UX for autocomplete is better. We can't change that now, parameters and credentials for example will always be a list, but we can use a map in Porter's code so that it's easier to work with. I've added ArrayEncodedMap, a data structure that reads in a list and stores it in-memory as a map: - name: a value: stuff - name: b value: things We can use this as a base data structure initially for Parameter and Credential Sets, storing the mapping of the parameter/credential name to the secret resolution strategy. Over time, it may make sense to use it for the other data structures in porter.yaml as well. It has support for iterating as a map, or as a sorted list of values. The sorted list is good for unit tests, and anytime we need the output to be stable. The map isn't exposed directly and I have a funky "best you can do with Go" iterator to help keep our for loops looking pretty normal. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs
added
the
pep003-advanced-dependencies
4 tasks
arschles
suggested changes
Apr 28, 2023
Contributor
arschles
left a comment
arschles
left a comment
There was a problem hiding this comment.
Looks good overall. I left a few questions
| // Use ItemsUnsafe() to directly manipulate the backing items map. | ||
| func (m *ArrayEncodedMap[T, K]) Items() map[string]T { | ||
| if m == nil { | ||
| return nil |
Contributor
There was a problem hiding this comment.
return an empty map here, so callers don't blow up?
Contributor
There was a problem hiding this comment.
Is this even reachable? If the object is nil then referencing any method on the object should result in a panic before it hits the implementation correct?
Contributor
There was a problem hiding this comment.
Never mind just clarified my own question. Go can call methods on nil structs :)
| return nil | ||
| } | ||
|
|
||
| if m.items == nil { |
Contributor
There was a problem hiding this comment.
is it ever possible to have items be nil?
| // UnmarshalRaw is the common Marshal implementation between YAML and JSON. | ||
| func (m *ArrayEncodedMap[T, K]) UnmarshalRaw(raw []K) error { | ||
| if m == nil { | ||
| *m = ArrayEncodedMap[T, K]{} |
Contributor
There was a problem hiding this comment.
won't this panic? seems like you'd need to set the value of the pointer to valid memory
Suggested change
| *m = ArrayEncodedMap[T, K]{} | |
| m = &ArrayEncodedMap[T, K]{} |
|
|
||
| // UnmarshalYAML unmarshals the items in the specified YAML. | ||
| func (m *ArrayEncodedMap[T, K]) UnmarshalYAML(value *yaml.Node) error { | ||
| var raw []K |
Contributor
There was a problem hiding this comment.
same here RE checking m == nil
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change
It's a common pattern in Porter to represent a map of items, with a unique key, as a list in the porter.yaml or a file representation of the resource because the UX for autocomplete is better. We can't change that now, parameters and credentials for example will always be a list, but we can use a map in Porter's code so that it's easier to work with.
I've added ArrayEncodedMap, a data structure that reads in a list and stores it in-memory as a map:
value: stuff
value: things
We can use this as a base data structure initially for Parameter and Credential Sets, storing the mapping of the parameter/credential name to the secret resolution strategy. Over time, it may make sense to use it for the other data structures in porter.yaml as well.
It has support for iterating as a map, or as a sorted list of values. The sorted list is good for unit tests, and anytime we need the output to be stable. The map isn't exposed directly and I have a funky "best you can do with Go" iterator to help keep our for loops looking pretty normal.
What issue does it fix
Refactoring work for PEP003 as I found working with parameter sets as maps made parameter resolution much more straightforward.
Notes for the reviewer
This is a replacement for my original PR #2734, which was adding helper methods to work with the set of values by key, but was still storing the values in a slice.
I will follow-up with another PR after this is merged to use it in ParameterSet/CredentialSet. You can see what that looks like in #2749. I wanted to give us a chance to bikeshed this first so that it's not super hard to rework with suggestions. Once we hook it up to ps/cs, the changeset gets pretty large.
Alternative names to consider... 😁
Checklist
Reviewer Checklist