Skip to content
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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 70 additions & 18 deletions pkg/secrets/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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!

Copy link
Member Author

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. 😂


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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
}

// GetResolvedValues converts the items to a map of key/value pairs, with the values represented as CNAB-compatible strings
func (l SourceMapList) GetResolvedValues() map[string]string {
carolynvs marked this conversation as resolved.
Show resolved Hide resolved
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
}
105 changes: 90 additions & 15 deletions pkg/secrets/strategy_test.go
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.GetResolvedValues()
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")
}
2 changes: 1 addition & 1 deletion pkg/storage/credentialset.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ type CredentialSetSpec struct {
Labels map[string]string `json:"labels,omitempty" yaml:"labels,omitempty" toml:"labels,omitempty"`

// Credentials is a list of credential resolution strategies.
Credentials secrets.StrategyList `json:"credentials" yaml:"credentials" toml:"credentials"`
Credentials secrets.SourceMapList `json:"credentials" yaml:"credentials" toml:"credentials"`
}

// CredentialSetStatus contains additional status metadata that has been set by Porter.
Expand Down
2 changes: 1 addition & 1 deletion pkg/storage/parameterset.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type ParameterSetSpec struct {
Labels map[string]string `json:"labels,omitempty" yaml:"labels,omitempty" toml:"labels,omitempty"`

// Parameters is a list of parameter specs.
Parameters secrets.StrategyList `json:"parameters" yaml:"parameters" toml:"parameters"`
Parameters secrets.SourceMapList `json:"parameters" yaml:"parameters" toml:"parameters"`
}

// ParameterSetStatus contains additional status metadata that has been set by Porter.
Expand Down