resourcemanager/resourceids: adding a Match function
#234
Conversation
The `Match` function takes two instances of the ResourceId type and allows comparing the parsed segments for these. Whilst this is also possible by comparing the value of `.ID()` in both instances - that's not case-aware and so will be problematic in the future - hence the need for this function which can account for case-aware comparisons. This also exposed a feature-flag for treating User Specified Segments as case-insensitive for comparison purposes - however this isn't usable at this time and so MUST NOT be exposed as a toggle at this point-in-time, since it'll cause more problems than it solves. However having this internal-only feature flag allows us to built out the surrounding components ahead-of-time and add tests covering both scenarios - therefore this is beneficial to have as an internal toggle today - even if it's not usable for end-users for some time.
There was a problem hiding this comment.
Thanks @tombuildsstuff I have two questions around the case insensitive flag, otherwise LGTM 👍
| // | ||
| // There are a number of dependencies to enabling this, including completing the standardiation on the | ||
| // `ResourceId` interface and the `ResourceIDReference` schema types - and surrounding updates. | ||
| var TreatUserSpecifiedSegmentsAsCaseInsensitive = false |
There was a problem hiding this comment.
Despite the comment I could still see this creeping into places where it shouldn't in the provider. Would it make sense and/or be possible for this to be accompanied by a check in the provider to flag new additions of this?
There was a problem hiding this comment.
By this, do you mean a check in the Provider to ensure that we're not doing .ID() ==? If so, yes - that can be added at the same time as fixing the 6 instances above
There was a problem hiding this comment.
I actually mean a check in the Provider to ensure we're not setting features.TreatUserSpecifiedSegmentsAsCaseInsensitive = true unless it's been explicitly added to a list of exceptions, like we do with these checks
There was a problem hiding this comment.
Yep, makes sense - we probably want both checks actually.
It's worth noting that since this feature-flag is for the behaviour as a whole (i.e. across the entire Provider) rather than being for individual Resource ID types - we'd only set this one-time rather than in multiple places anyway.
There was a problem hiding this comment.
For info: I've intentionally not included the validation changes in hashicorp/terraform-provider-azurerm#25873 since this wants to be done in a follow up PR - will send that in the morning:
The
Matchfunction takes two instances of the ResourceId type and allows comparing the parsed segments for these.Whilst this is also possible by comparing the value of
.ID()in both instances - that's not case-aware and so will be problematic in the future - hence the need for this function which can account for case-aware comparisons.This also exposed a feature-flag for treating User Specified Segments as case-insensitive for comparison purposes - however this isn't usable at this time and so must not be exposed as a toggle at this point-in-time, since it'll cause more problems than it solves. However having this internal-only feature flag allows us to built out the surrounding components ahead-of-time and add tests covering both scenarios - therefore this is beneficial to have as an internal toggle today - even if it's not usable for end-users for some time.
There are currently 6 instances of this across the Provider:
in addition to (at least) hashicorp/terraform-provider-azurerm#25809 - therefore we should get ahead of this and switch to using
resourceids.Matchto enable us to handle this consistently in the future.