Add support for dynamic resource id recasing #221
Conversation
resourcemanager/commonids/ids.go
Outdated
| &DevCenterId{}, | ||
| &DiskEncryptionSetId{}, | ||
| &ExpressRouteCircuitPeeringId{}, | ||
| &FleetId{}, |
| "github.com/hashicorp/go-azure-helpers/resourcemanager/recaser" | ||
| ) | ||
|
|
||
| func TestRecaserWithIncorrectCasing(t *testing.T) { |
There was a problem hiding this comment.
could we add a couple of the awkward test cases:
- There's a handful of APIs that return the full URI with a port in some cases e.g.
https://management.azure.com:80/subscriptions/12345 - Could we add a Data Plane URI here to ensure this is ignored too:
https://example.blob.storage.azure.com/container1should be fine
As such could we add a couple of test cases covering those?
| resourceIdsWriteLock.Lock() | ||
| for _, id := range commonids.CommonIds() { | ||
| key := strings.ToLower(id.ID()) | ||
| if _, ok := knownResourceIds[key]; !ok { | ||
| knownResourceIds[key] = id | ||
| } | ||
| } | ||
| resourceIdsWriteLock.Unlock() |
There was a problem hiding this comment.
perhaps pedantic, but is there a reason not to call RegisterResourceId here instead?
| resourceIdsWriteLock.Lock() | |
| for _, id := range commonids.CommonIds() { | |
| key := strings.ToLower(id.ID()) | |
| if _, ok := knownResourceIds[key]; !ok { | |
| knownResourceIds[key] = id | |
| } | |
| } | |
| resourceIdsWriteLock.Unlock() | |
| for _, id := range commonids.CommonIds() { | |
| RegisterResourceId(id) | |
| } |
There was a problem hiding this comment.
makes sense 👍 fixed now!
resourcemanager/recaser/recase.go
Outdated
| // ReCaseWithIds tries to determine the type of Resource ID defined in `input` to be able to re-case it from based on an input list of Resource IDs | ||
| func ReCaseWithIds(input string, ids map[string]resourceids.ResourceId) string { |
There was a problem hiding this comment.
Can we make this function private/unexported? This should only be used for testing purposes, else we might end up using this unintentionally (which would be a bug, since it'd mean we're not registering all of the Resource IDs)
| // ReCaseWithIds tries to determine the type of Resource ID defined in `input` to be able to re-case it from based on an input list of Resource IDs | |
| func ReCaseWithIds(input string, ids map[string]resourceids.ResourceId) string { | |
| // reCaseWithIds tries to determine the type of Resource ID defined in `input` to be able to re-case it from based on an input list of Resource IDs | |
| func reCaseWithIds(input string, ids map[string]resourceids.ResourceId) string { |
resourcemanager/recaser/recase.go
Outdated
|
|
||
| // buildInputKey takes an input id string and removes user-specified values from it | ||
| // so it can be used as a key to extract the correct id from knownResourceIds | ||
| func buildInputKey(input string) string { |
There was a problem hiding this comment.
perhaps we should update this to also return a boolean for whether this is usable or not?
| func buildInputKey(input string) string { | |
| func buildInputKey(input string) (*string, bool) { |
That'd allow this to become:
key, ok := buildInputKey(input)
if ok {
// assuming that `key` had a value so *key should be safe
}
| output = fmt.Sprintf("%s/%s/", output, key) | ||
| if strings.EqualFold(key, "providers") && len(segments) >= i+2 { | ||
| value := segments[i+1] | ||
| output = fmt.Sprintf("%s%s", output, value) |
There was a problem hiding this comment.
perhaps it's worth being consistent here?
| output = fmt.Sprintf("%s%s", output, value) | |
| output = fmt.Sprintf("%s/%s", output, value) |
There was a problem hiding this comment.
in this case we don't need to add / since there is no user input between providers and the next segment. I've added some comments to make the expected inputs and outputs clearer here
resourcemanager/recaser/recase.go
Outdated
| output = fmt.Sprintf("%s/%s/", output, key) | ||
| if strings.EqualFold(key, "providers") && len(segments) >= i+2 { |
There was a problem hiding this comment.
might be worth a comment explaining the why here?
| "github.com/hashicorp/go-azure-helpers/resourcemanager/resourceids" | ||
| ) | ||
|
|
||
| var KnownResourceIds = make(map[string]resourceids.ResourceId) |
There was a problem hiding this comment.
Out of interest, what's the reason for having this public/exported?
There was a problem hiding this comment.
the compiler was giving an error, but you're right it looks like it doesn't really need to be public - fixed
There was a problem hiding this comment.
Thanks @catriona-m. One minor suggestion on the function comment but otherwise LGTM 👍
Co-authored-by: stephybun <steph@hashicorp.com>
partially addresses #189