From 0d5d587b1731e27b987bf5cde2ae5d973c0110ba Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 14 Nov 2024 15:32:29 +0000 Subject: [PATCH 1/3] feat(provisionersdk): allow variadic tags in provisionersdk.MutateTags --- provisionersdk/provisionertags.go | 28 +++++- provisionersdk/provisionertags_test.go | 131 ++++++++++++++++++++----- 2 files changed, 132 insertions(+), 27 deletions(-) diff --git a/provisionersdk/provisionertags.go b/provisionersdk/provisionertags.go index 741fbeede279d..dd16723fae44d 100644 --- a/provisionersdk/provisionertags.go +++ b/provisionersdk/provisionertags.go @@ -14,14 +14,16 @@ const ( // If the scope is "user", the "owner" is changed to the user ID. // This is for user-scoped provisioner daemons, where users should // own their own operations. +// Multiple sets of tags may be passed to this function; they will +// be merged into one single tag set. // Otherwise, the "owner" tag is always an empty string. // NOTE: "owner" must NEVER be nil. Otherwise it will end up being // duplicated in the database, as idx_provisioner_daemons_name_owner_key // is a partial unique index that includes a JSON field. -func MutateTags(userID uuid.UUID, provided map[string]string) map[string]string { +func MutateTags(userID uuid.UUID, provided ...map[string]string) map[string]string { tags := map[string]string{} - for k, v := range provided { - tags[k] = v + for _, m := range provided { + tags = mergeTags(tags, m) } _, ok := tags[TagScope] if !ok { @@ -39,3 +41,23 @@ func MutateTags(userID uuid.UUID, provided map[string]string) map[string]string } return tags } + +// mergeTags merges multiple sets of provisioner tags. +// Given maps a and b, the result will contain all +// keys and values contained in a and in b. +// An exception is made for key-value pairs for which +// a[key] is non-empty but b[key] is empty. In this case +// the non-empty value from a will win. +func mergeTags(a, b map[string]string) map[string]string { + m := make(map[string]string) + for k, v := range a { + m[k] = v + } + for k, v := range b { + if v == "" { + continue + } + m[k] = v + } + return m +} diff --git a/provisionersdk/provisionertags_test.go b/provisionersdk/provisionertags_test.go index 911de161c1eae..11c80991aab98 100644 --- a/provisionersdk/provisionertags_test.go +++ b/provisionersdk/provisionertags_test.go @@ -1,7 +1,6 @@ package provisionersdk_test import ( - "encoding/json" "testing" "github.com/coder/coder/v2/provisionersdk" @@ -18,13 +17,13 @@ func TestMutateTags(t *testing.T) { for _, tt := range []struct { name string userID uuid.UUID - tags map[string]string + tags []map[string]string want map[string]string }{ { name: "nil tags", userID: uuid.Nil, - tags: nil, + tags: mapslice(nil), want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: "", @@ -33,15 +32,17 @@ func TestMutateTags(t *testing.T) { { name: "empty tags", userID: uuid.Nil, - tags: map[string]string{}, + tags: mapslice(map[string]string{}), want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: "", }, }, { - name: "user scope", - tags: map[string]string{provisionersdk.TagScope: provisionersdk.ScopeUser}, + name: "user scope", + tags: mapslice( + map[string]string{provisionersdk.TagScope: provisionersdk.ScopeUser}, + ), userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeUser, @@ -49,8 +50,10 @@ func TestMutateTags(t *testing.T) { }, }, { - name: "organization scope", - tags: map[string]string{provisionersdk.TagScope: provisionersdk.ScopeOrganization}, + name: "organization scope", + tags: mapslice( + map[string]string{provisionersdk.TagScope: provisionersdk.ScopeOrganization}, + ), userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -59,10 +62,25 @@ func TestMutateTags(t *testing.T) { }, { name: "organization scope with owner", - tags: map[string]string{ + tags: mapslice( + map[string]string{ + provisionersdk.TagScope: provisionersdk.ScopeOrganization, + provisionersdk.TagOwner: testUserID.String(), + }, + ), + userID: uuid.Nil, + want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, - provisionersdk.TagOwner: testUserID.String(), + provisionersdk.TagOwner: "", }, + }, + { + name: "owner tag with no other context", + tags: mapslice( + map[string]string{ + provisionersdk.TagOwner: testUserID.String(), + }, + ), userID: uuid.Nil, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -70,38 +88,103 @@ func TestMutateTags(t *testing.T) { }, }, { - name: "owner tag with no other context", - tags: map[string]string{ - provisionersdk.TagOwner: testUserID.String(), + name: "invalid scope", + tags: mapslice( + map[string]string{provisionersdk.TagScope: "360noscope"}, + ), + userID: testUserID, + want: map[string]string{ + provisionersdk.TagScope: provisionersdk.ScopeOrganization, + provisionersdk.TagOwner: "", }, - userID: uuid.Nil, + }, + { + name: "merge two empty maps", + tags: mapslice( + map[string]string{}, + map[string]string{}, + ), + userID: testUserID, + want: map[string]string{ + provisionersdk.TagScope: provisionersdk.ScopeOrganization, + provisionersdk.TagOwner: "", + }, + }, + { + name: "merge empty map with non-empty map", + tags: mapslice( + map[string]string{}, + map[string]string{"foo": "bar"}, + ), + userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: "", + "foo": "bar", }, }, { - name: "invalid scope", - tags: map[string]string{provisionersdk.TagScope: "360noscope"}, + name: "merge non-empty map with empty map", + tags: mapslice( + map[string]string{"foo": "bar"}, + map[string]string{}, + ), userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: "", + "foo": "bar", + }, + }, + { + name: "merge map with same map", + tags: mapslice( + map[string]string{"foo": "bar"}, + map[string]string{"foo": "bar"}, + ), + userID: testUserID, + want: map[string]string{ + provisionersdk.TagScope: provisionersdk.ScopeOrganization, + provisionersdk.TagOwner: "", + "foo": "bar", + }, + }, + { + name: "merge map with override", + tags: mapslice( + map[string]string{"foo": "bar"}, + map[string]string{"foo": "baz"}, + ), + userID: testUserID, + want: map[string]string{ + provisionersdk.TagScope: provisionersdk.ScopeOrganization, + provisionersdk.TagOwner: "", + "foo": "baz", + }, + }, + { + name: "do not override empty in second map", + tags: mapslice( + map[string]string{"foo": "bar"}, + map[string]string{"foo": ""}, + ), + userID: testUserID, + want: map[string]string{ + provisionersdk.TagScope: provisionersdk.ScopeOrganization, + provisionersdk.TagOwner: "", + "foo": "bar", }, }, } { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - // make a copy of the map because the function under test - // mutates the map - bytes, err := json.Marshal(tt.tags) - require.NoError(t, err) - var tags map[string]string - err = json.Unmarshal(bytes, &tags) - require.NoError(t, err) - got := provisionersdk.MutateTags(tt.userID, tags) + got := provisionersdk.MutateTags(tt.userID, tt.tags...) require.Equal(t, tt.want, got) }) } } + +func mapslice(m ...map[string]string) []map[string]string { + return m +} From 905e66e057c1f7ae1973e4add3dce1894e182533 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 14 Nov 2024 15:36:58 +0000 Subject: [PATCH 2/3] i keep forgetting about util/slice --- provisionersdk/provisionertags_test.go | 31 ++++++++++++-------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/provisionersdk/provisionertags_test.go b/provisionersdk/provisionertags_test.go index 11c80991aab98..356791448b91f 100644 --- a/provisionersdk/provisionertags_test.go +++ b/provisionersdk/provisionertags_test.go @@ -3,6 +3,7 @@ package provisionersdk_test import ( "testing" + "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/provisionersdk" "github.com/google/uuid" @@ -23,7 +24,7 @@ func TestMutateTags(t *testing.T) { { name: "nil tags", userID: uuid.Nil, - tags: mapslice(nil), + tags: []map[string]string{nil}, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: "", @@ -32,7 +33,7 @@ func TestMutateTags(t *testing.T) { { name: "empty tags", userID: uuid.Nil, - tags: mapslice(map[string]string{}), + tags: slice.New(map[string]string{}), want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: "", @@ -40,7 +41,7 @@ func TestMutateTags(t *testing.T) { }, { name: "user scope", - tags: mapslice( + tags: slice.New( map[string]string{provisionersdk.TagScope: provisionersdk.ScopeUser}, ), userID: testUserID, @@ -51,7 +52,7 @@ func TestMutateTags(t *testing.T) { }, { name: "organization scope", - tags: mapslice( + tags: slice.New( map[string]string{provisionersdk.TagScope: provisionersdk.ScopeOrganization}, ), userID: testUserID, @@ -62,7 +63,7 @@ func TestMutateTags(t *testing.T) { }, { name: "organization scope with owner", - tags: mapslice( + tags: slice.New( map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: testUserID.String(), @@ -76,7 +77,7 @@ func TestMutateTags(t *testing.T) { }, { name: "owner tag with no other context", - tags: mapslice( + tags: slice.New( map[string]string{ provisionersdk.TagOwner: testUserID.String(), }, @@ -89,7 +90,7 @@ func TestMutateTags(t *testing.T) { }, { name: "invalid scope", - tags: mapslice( + tags: slice.New( map[string]string{provisionersdk.TagScope: "360noscope"}, ), userID: testUserID, @@ -100,7 +101,7 @@ func TestMutateTags(t *testing.T) { }, { name: "merge two empty maps", - tags: mapslice( + tags: slice.New( map[string]string{}, map[string]string{}, ), @@ -112,7 +113,7 @@ func TestMutateTags(t *testing.T) { }, { name: "merge empty map with non-empty map", - tags: mapslice( + tags: slice.New( map[string]string{}, map[string]string{"foo": "bar"}, ), @@ -125,7 +126,7 @@ func TestMutateTags(t *testing.T) { }, { name: "merge non-empty map with empty map", - tags: mapslice( + tags: slice.New( map[string]string{"foo": "bar"}, map[string]string{}, ), @@ -138,7 +139,7 @@ func TestMutateTags(t *testing.T) { }, { name: "merge map with same map", - tags: mapslice( + tags: slice.New( map[string]string{"foo": "bar"}, map[string]string{"foo": "bar"}, ), @@ -151,7 +152,7 @@ func TestMutateTags(t *testing.T) { }, { name: "merge map with override", - tags: mapslice( + tags: slice.New( map[string]string{"foo": "bar"}, map[string]string{"foo": "baz"}, ), @@ -164,7 +165,7 @@ func TestMutateTags(t *testing.T) { }, { name: "do not override empty in second map", - tags: mapslice( + tags: slice.New( map[string]string{"foo": "bar"}, map[string]string{"foo": ""}, ), @@ -184,7 +185,3 @@ func TestMutateTags(t *testing.T) { }) } } - -func mapslice(m ...map[string]string) []map[string]string { - return m -} From 70de358c5ee6b19e99d2f1b7f394c91c8aebf155 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 15 Nov 2024 12:15:16 +0000 Subject: [PATCH 3/3] address PR comments --- provisionersdk/provisionertags.go | 9 +-- provisionersdk/provisionertags_test.go | 90 ++++++++++++++------------ 2 files changed, 52 insertions(+), 47 deletions(-) diff --git a/provisionersdk/provisionertags.go b/provisionersdk/provisionertags.go index dd16723fae44d..a3bcb68df3b26 100644 --- a/provisionersdk/provisionertags.go +++ b/provisionersdk/provisionertags.go @@ -42,12 +42,9 @@ func MutateTags(userID uuid.UUID, provided ...map[string]string) map[string]stri return tags } -// mergeTags merges multiple sets of provisioner tags. -// Given maps a and b, the result will contain all -// keys and values contained in a and in b. -// An exception is made for key-value pairs for which -// a[key] is non-empty but b[key] is empty. In this case -// the non-empty value from a will win. +// mergeTags merges two sets of provisioner tags. +// If b[key] is an empty string, the value from a[key] is retained. +// This function handles nil maps gracefully. func mergeTags(a, b map[string]string) map[string]string { m := make(map[string]string) for k, v := range a { diff --git a/provisionersdk/provisionertags_test.go b/provisionersdk/provisionertags_test.go index 356791448b91f..70e05473eecc0 100644 --- a/provisionersdk/provisionertags_test.go +++ b/provisionersdk/provisionertags_test.go @@ -3,7 +3,6 @@ package provisionersdk_test import ( "testing" - "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/provisionersdk" "github.com/google/uuid" @@ -33,7 +32,7 @@ func TestMutateTags(t *testing.T) { { name: "empty tags", userID: uuid.Nil, - tags: slice.New(map[string]string{}), + tags: []map[string]string{{}}, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: "", @@ -41,9 +40,9 @@ func TestMutateTags(t *testing.T) { }, { name: "user scope", - tags: slice.New( - map[string]string{provisionersdk.TagScope: provisionersdk.ScopeUser}, - ), + tags: []map[string]string{ + {provisionersdk.TagScope: provisionersdk.ScopeUser}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeUser, @@ -52,9 +51,9 @@ func TestMutateTags(t *testing.T) { }, { name: "organization scope", - tags: slice.New( - map[string]string{provisionersdk.TagScope: provisionersdk.ScopeOrganization}, - ), + tags: []map[string]string{ + {provisionersdk.TagScope: provisionersdk.ScopeOrganization}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -63,12 +62,12 @@ func TestMutateTags(t *testing.T) { }, { name: "organization scope with owner", - tags: slice.New( - map[string]string{ + tags: []map[string]string{ + { provisionersdk.TagScope: provisionersdk.ScopeOrganization, provisionersdk.TagOwner: testUserID.String(), }, - ), + }, userID: uuid.Nil, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -77,11 +76,11 @@ func TestMutateTags(t *testing.T) { }, { name: "owner tag with no other context", - tags: slice.New( - map[string]string{ + tags: []map[string]string{ + { provisionersdk.TagOwner: testUserID.String(), }, - ), + }, userID: uuid.Nil, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -90,9 +89,9 @@ func TestMutateTags(t *testing.T) { }, { name: "invalid scope", - tags: slice.New( - map[string]string{provisionersdk.TagScope: "360noscope"}, - ), + tags: []map[string]string{ + {provisionersdk.TagScope: "360noscope"}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -101,10 +100,10 @@ func TestMutateTags(t *testing.T) { }, { name: "merge two empty maps", - tags: slice.New( - map[string]string{}, - map[string]string{}, - ), + tags: []map[string]string{ + {}, + {}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -113,10 +112,10 @@ func TestMutateTags(t *testing.T) { }, { name: "merge empty map with non-empty map", - tags: slice.New( - map[string]string{}, - map[string]string{"foo": "bar"}, - ), + tags: []map[string]string{ + {}, + {"foo": "bar"}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -126,10 +125,10 @@ func TestMutateTags(t *testing.T) { }, { name: "merge non-empty map with empty map", - tags: slice.New( - map[string]string{"foo": "bar"}, - map[string]string{}, - ), + tags: []map[string]string{ + {"foo": "bar"}, + {}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -139,10 +138,10 @@ func TestMutateTags(t *testing.T) { }, { name: "merge map with same map", - tags: slice.New( - map[string]string{"foo": "bar"}, - map[string]string{"foo": "bar"}, - ), + tags: []map[string]string{ + {"foo": "bar"}, + {"foo": "bar"}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -152,10 +151,10 @@ func TestMutateTags(t *testing.T) { }, { name: "merge map with override", - tags: slice.New( - map[string]string{"foo": "bar"}, - map[string]string{"foo": "baz"}, - ), + tags: []map[string]string{ + {"foo": "bar"}, + {"foo": "baz"}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -165,10 +164,10 @@ func TestMutateTags(t *testing.T) { }, { name: "do not override empty in second map", - tags: slice.New( - map[string]string{"foo": "bar"}, - map[string]string{"foo": ""}, - ), + tags: []map[string]string{ + {"foo": "bar"}, + {"foo": ""}, + }, userID: testUserID, want: map[string]string{ provisionersdk.TagScope: provisionersdk.ScopeOrganization, @@ -176,6 +175,15 @@ func TestMutateTags(t *testing.T) { "foo": "bar", }, }, + { + name: "merge nil map with nil map", + tags: []map[string]string{nil, nil}, + userID: testUserID, + want: map[string]string{ + provisionersdk.TagScope: provisionersdk.ScopeOrganization, + provisionersdk.TagOwner: "", + }, + }, } { tt := tt t.Run(tt.name, func(t *testing.T) {