-
Notifications
You must be signed in to change notification settings - Fork 212
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
Add functionality to SourceMapList #2734
Changes from all commits
edc822f
8ab663e
762fc59
0ec0322
23034f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,20 +13,6 @@ import ( | |
// This is the output of resolving a parameter or credential set file. | ||
type Set map[string]string | ||
|
||
// Merge merges a second Set into the base. | ||
// | ||
// Duplicate names are not allow and will result in an | ||
// error, this is the case even if the values are identical. | ||
func (s Set) Merge(s2 Set) error { | ||
for k, v := range s2 { | ||
if _, ok := s[k]; ok { | ||
return fmt.Errorf("ambiguous value resolution: %q is already present in base sets, cannot merge", k) | ||
} | ||
s[k] = v | ||
} | ||
return nil | ||
} | ||
|
||
// IsValid determines if the provided key (designating a name of a parameter | ||
// or credential) is included in the provided set | ||
func (s Set) IsValid(key string) bool { | ||
|
@@ -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 | ||
|
||
func (l StrategyList) Less(i, j int) bool { | ||
func (l SourceMapList) Less(i, j int) bool { | ||
return l[i].Name < l[j].Name | ||
} | ||
|
||
func (l StrategyList) Swap(i, j int) { | ||
func (l SourceMapList) Swap(i, j int) { | ||
tmp := l[i] | ||
l[i] = l[j] | ||
l[j] = tmp | ||
} | ||
|
||
func (l StrategyList) Len() int { | ||
func (l SourceMapList) Len() int { | ||
return len(l) | ||
} | ||
|
||
// HasName determines if the specified name is defined in the set. | ||
func (l SourceMapList) HasName(name string) bool { | ||
_, ok := l.GetByName(name) | ||
return ok | ||
} | ||
|
||
// GetByName returns the resolution strategy for the specified name and a bool indicating if it was found. | ||
func (l SourceMapList) GetByName(name string) (SourceMap, bool) { | ||
carolynvs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for _, item := range l { | ||
if item.Name == name { | ||
return item, true | ||
} | ||
} | ||
|
||
return SourceMap{}, false | ||
} | ||
|
||
// 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 not do resolution for you. | ||
func (l SourceMapList) GetResolvedValue(name string) (interface{}, bool) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you use a type param like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"). |
||
item, ok := l.GetByName(name) | ||
if ok { | ||
return item.ResolvedValue, true | ||
} | ||
|
||
return nil, false | ||
} | ||
|
||
// ToResolvedValues converts the items to a map of key/value pairs, with the resolved values represented as CNAB-compatible strings | ||
func (l SourceMapList) ToResolvedValues() map[string]string { | ||
values := make(map[string]string, len(l)) | ||
for _, item := range l { | ||
values[item.Name] = item.ResolvedValue | ||
} | ||
return values | ||
} | ||
|
||
// Merge applies the specified values on top of a base set of values. When a | ||
// name exists in both sets, use the value from the overrides | ||
func (l SourceMapList) Merge(overrides SourceMapList) SourceMapList { | ||
result := l | ||
|
||
// Make a lookup from the name to its index in result so that we can quickly find a | ||
// named item while merging | ||
lookup := make(map[string]int, len(result)) | ||
for i, item := range result { | ||
lookup[item.Name] = i | ||
} | ||
|
||
for _, item := range overrides { | ||
// If the name is in the base, overwrite its value with the override provided | ||
if i, ok := lookup[item.Name]; ok { | ||
result[i].Source = item.Source | ||
|
||
// Just in case the value was resolved, include in the merge results | ||
result[i].ResolvedValue = item.ResolvedValue | ||
} else { | ||
// Append the override to the list of results if it's not in the base set of values | ||
result = append(result, item) | ||
} | ||
} | ||
|
||
return result | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,30 +1,105 @@ | ||
package secrets | ||
|
||
import ( | ||
"sort" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestSet_Merge(t *testing.T) { | ||
set := Set{ | ||
"first": "first", | ||
"second": "second", | ||
"third": "third", | ||
func TestSourceMapList_GetResolvedValue(t *testing.T) { | ||
l := SourceMapList{ | ||
SourceMap{Name: "bar", ResolvedValue: "2"}, | ||
SourceMap{Name: "foo", ResolvedValue: "1"}, | ||
} | ||
|
||
fooVal, ok := l.GetResolvedValue("foo") | ||
require.True(t, ok, "foo could not be found") | ||
assert.Equal(t, "1", fooVal, "foo had the incorrect resolved value") | ||
|
||
_, ok = l.GetResolvedValue("missing") | ||
require.False(t, ok, "GetResolvedValue should have returned that missing key was not found") | ||
} | ||
|
||
func TestSourceMapList_GetResolvedValues(t *testing.T) { | ||
l := SourceMapList{ | ||
SourceMap{Name: "bar", ResolvedValue: "2"}, | ||
SourceMap{Name: "foo", ResolvedValue: "1"}, | ||
} | ||
|
||
want := map[string]string{ | ||
"bar": "2", | ||
"foo": "1", | ||
} | ||
got := l.ToResolvedValues() | ||
assert.Equal(t, want, got) | ||
} | ||
|
||
func TestSourceMapList_HasKey(t *testing.T) { | ||
l := SourceMapList{ | ||
SourceMap{Name: "bar", ResolvedValue: "2"}, | ||
SourceMap{Name: "foo", ResolvedValue: "1"}, | ||
} | ||
|
||
require.True(t, l.HasName("foo"), "HasName returned the wrong value for a key present in the list") | ||
require.False(t, l.HasName("missing"), "HasName returned the wrong value for a missing key") | ||
} | ||
|
||
func TestSourceMapList_Sort(t *testing.T) { | ||
l := SourceMapList{ | ||
SourceMap{Name: "c"}, | ||
SourceMap{Name: "a"}, | ||
SourceMap{Name: "b"}, | ||
} | ||
|
||
sort.Sort(l) | ||
|
||
wantResult := SourceMapList{ | ||
SourceMap{Name: "a"}, | ||
SourceMap{Name: "b"}, | ||
SourceMap{Name: "c"}, | ||
} | ||
|
||
require.Len(t, l, 3, "Len is not implemented correctly") | ||
require.Equal(t, wantResult, l, "Sort is not implemented correctly") | ||
} | ||
|
||
func TestSourceMapList_GetStrategy(t *testing.T) { | ||
wantToken := SourceMap{Name: "token", Source: Source{Strategy: "env", Hint: "GITHUB_TOKEN"}} | ||
l := SourceMapList{ | ||
SourceMap{Name: "logLevel", Source: Source{Strategy: "value", Hint: "11"}}, | ||
wantToken, | ||
} | ||
|
||
gotToken, ok := l.GetByName("token") | ||
require.True(t, ok, "GetByName did not find 'token'") | ||
assert.Equal(t, wantToken, gotToken, "GetByName returned the wrong 'token' strategy") | ||
} | ||
|
||
func TestSourceMapList_Merge(t *testing.T) { | ||
set := SourceMapList{ | ||
SourceMap{Name: "first", ResolvedValue: "base first"}, | ||
SourceMap{Name: "second", ResolvedValue: "base second"}, | ||
SourceMap{Name: "third", ResolvedValue: "base third"}, | ||
} | ||
|
||
is := assert.New(t) | ||
|
||
err := set.Merge(Set{}) | ||
is.NoError(err) | ||
is.Len(set, 3) | ||
is.NotContains(set, "fourth") | ||
result := set.Merge(SourceMapList{}) | ||
is.Len(result, 3) | ||
_, ok := result.GetResolvedValue("base fourth") | ||
is.False(ok, "forth should not be present in the base set") | ||
|
||
err = set.Merge(Set{"fourth": "fourth"}) | ||
is.NoError(err) | ||
is.Len(set, 4) | ||
is.Contains(set, "fourth") | ||
result = set.Merge(SourceMapList{SourceMap{Name: "fourth", ResolvedValue: "new fourth"}}) | ||
is.Len(result, 4) | ||
val, ok := result.GetResolvedValue("fourth") | ||
is.True(ok, "fourth should be present when merged as an override") | ||
is.Equal("new fourth", val, "incorrect merged value") | ||
|
||
err = set.Merge(Set{"second": "bis"}) | ||
is.EqualError(err, `ambiguous value resolution: "second" is already present in base sets, cannot merge`) | ||
result = set.Merge(SourceMapList{SourceMap{Name: "second", ResolvedValue: "override second"}}) | ||
is.Len(result, 3) | ||
val, ok = result.GetResolvedValue("second") | ||
is.True(ok, "second should be overwritten when an override value is merged") | ||
is.Equal("override second", val, "incorrect merged value") | ||
} |
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.
Should there be a name safe "Add" method?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😂