azurerm_network_interface & azurerm_arm_loadbalancer: fixing d.Set() set errors and some additional validation #1403
Conversation
azurerm/helpers/validate/azure.go
Outdated
| //todo, match subscription/{guid} and provider/name.name perhaps? | ||
| if matched := regexp.MustCompile(`(/subscriptions/)|(/providers/)`).Match([]byte(v)); !matched { | ||
| errors = append(errors, fmt.Errorf("azure resource ID %q should start with with '/subscriptions/{subscriptionId}' or '/providers/{resourceProviderNamespace}/'", k)) | ||
| } |
There was a problem hiding this comment.
IMO we'd be better to just call parseAzureResourceID here?
There was a problem hiding this comment.
tenants are also a possible prefix too e.g. /tenants/000/subscriptions/000/...
| _, errors := Ip4Address(tc.Ip, "test") | ||
|
|
||
| if len(errors) < tc.Errors { | ||
| t.Fatalf("Expected AzureResourceId to have an error for '%q'", tc.Ip) |
There was a problem hiding this comment.
minor technically '%q' can be just %q
azurerm/resource_arm_loadbalancer.go
Outdated
| Required: true, | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ValidateFunc: validate.StringNotEmpty, |
There was a problem hiding this comment.
there's a generic validation function in Core for "no default value" which may be better to use?
There was a problem hiding this comment.
I didn't know about that one! validation.NoZeroValues is perfect.
| return idObj, nil | ||
| } | ||
|
|
||
| func composeAzureResourceID(idObj *ResourceID) (id string, err error) { |
There was a problem hiding this comment.
I don't see anything using this apart from the Tests, in which case can we remove this?
There was a problem hiding this comment.
I removed the top level one, and kept this and the tests here on the new function
There was a problem hiding this comment.
sorry, I meant since this is Private and it's not being used within this Package, I think this is dead code?
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: true, | ||
| ValidateFunc: azure.ValidateResourceId, |
There was a problem hiding this comment.
this is an IP Address, not a Resource ID?
There was a problem hiding this comment.
Whoops, got copy paste happy it seems
| // into a ResourceID. We make assumptions about the structure of URLs, | ||
| // which is obviously not good, but the best thing available given the | ||
| // SDK. | ||
| func ParseAzureResourceID(id string) (*ResourceID, error) { |
There was a problem hiding this comment.
are we also going to call this method from the existing method to give us a migration path whilst refactoring?
There was a problem hiding this comment.
Thats a good idea, now threading down to new method
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
a result of investigating #1318