Add V2 exported services config support #399
Conversation
skpratt
force-pushed
the
v2-exported-services-config-v1-api
branch
2 times, most recently
from
17dc7f7
to
bbc841b
Compare
skpratt
force-pushed
the
v2-exported-services-config-v1-api
branch
from
bbc841b
to
6559cb7
Compare
skpratt
force-pushed
the
v2-exported-services-config-v1-api
branch
from
15c102b
to
8cd6162
Compare
skpratt
force-pushed
the
v2-exported-services-config-v1-api
branch
from
8cd6162
to
6f06e0c
Compare
|
@absolutelightning looks like I cannot add you as a reviewer |
|
@absolutelightning I confirmed with IT that your team should now all have reviewer capabilities |
Sure I will review tomorrow. |
| services := d.Get("services").([]interface{}) | ||
| if len(services) > 0 { | ||
| data["services"] = services | ||
| } |
There was a problem hiding this comment.
Should we return if data["services"] is blank?
There was a problem hiding this comment.
no, because NamespaceExportedServices and PartitionExportedServices do not explicitly set services
| Kind: kind, | ||
| } | ||
| var consumers []map[string]any | ||
| peerConsumers := d.Get("peer_consumers").([]interface{}) |
There was a problem hiding this comment.
All the Get calls with d.Get will fail of user has not given the key in schema. From the documentation
// Get returns the data for the given key, or nil if the key doesn't exist
// in the schema.
Since these are optional keys we need to add nil checks everywhere.
There was a problem hiding this comment.
refactored to in-line the loop - this pattern is used with optional keys everywhere
| } | ||
|
|
||
| func v2MulticlusterRead(client *api.Client, gvk *GVK, resourceName string, q *api.QueryOptions) (map[string]interface{}, error) { | ||
| endpoint := strings.ToLower(fmt.Sprintf("/api/%s/%s/%s/%s", gvk.Group, gvk.Version, gvk.Kind, resourceName)) |
There was a problem hiding this comment.
Are we converting the resourceNameto lower case as well?
Is this intended?
There was a problem hiding this comment.
We had an ADR about this to make all resource names lower case https://docs.google.com/document/d/1F8EZ76l9FaMnupJ4jCAw_42RLWPGO7oOZHfLs4TLutg/edit
There was a problem hiding this comment.
Thank you for adding this @skpratt ! I had few questions below but nothing blocking on my side
| @@ -74,7 +74,7 @@ func TestAccConsulACLBindingRule_basic(t *testing.T) { | |||
| }, | |||
| { | |||
| Config: testResourceACLBindingRuleConfig_wrongType, | |||
| ExpectError: regexp.MustCompile(`Invalid Binding Rule: unknown BindType "foobar"`), | |||
| ExpectError: regexp.MustCompile(`unknown BindType "foobar"`), | |||
There was a problem hiding this comment.
Is the change to this check related to introducing v2 exported services?
There was a problem hiding this comment.
The error message changed slightly with the version updates in .github/workflows/test.yaml, and the test started to fail
| } | ||
|
|
||
| func v2MulticlusterRead(client *api.Client, gvk *GVK, resourceName string, q *api.QueryOptions) (map[string]interface{}, error) { | ||
| endpoint := strings.ToLower(fmt.Sprintf("/api/%s/%s/%s/%s", gvk.Group, gvk.Version, gvk.Kind, resourceName)) |
There was a problem hiding this comment.
We had an ADR about this to make all resource names lower case https://docs.google.com/document/d/1F8EZ76l9FaMnupJ4jCAw_42RLWPGO7oOZHfLs4TLutg/edit
skpratt
force-pushed
the
v2-exported-services-config-v1-api
branch
from
c1a68b7
to
8fdc405
Compare
uses the small api module change merged here: hashicorp/consul#20737