API Group & Kind configuration for Terraformed resources #118
Conversation
ulucinar
force-pushed
the
fix-108
branch
3 times, most recently
from
c90d185 to
8dd2520
Compare
| // This file is generated | ||
| // nolint:misspell | ||
| var ( | ||
| apiGroupMap = map[string]string{ |
There was a problem hiding this comment.
In Jet AWS and Jet GCP, we opted for a pattern where we selectively keep using the words that were considered as group with ReplaceGroupWords function. For example, if aws_ebs_volume should have ec2 group, we still want to keep ebs word, hence try to get to EBSVolume as kind in ec2 group. Would it be feasible to do a similar thing here as well to reduce number of kind overrides?
There was a problem hiding this comment.
It turns out Terraform native Azure provider does not have a flat structure like the native AWS provider. It uses more structure (e.g., interface implementations) for registering resources. Thus, it won't be as straightforward as the AWS version to implement a scheme based on the Terraform package names, though it should still be doable.
But as a defaulting mechanism I also liked the idea of relying on the grouping chosen by Microsoft as discussed here because of low maintenance cost and high consistency with the native (REST) implementation. So we can keep these native provider names as the defaults and provide any manual override configuration under config/<package>.
There was a problem hiding this comment.
What I meant was that let's say we have a resource aws_db_instance whose group is overridden by us as rds. So, if we don't override its kind it will be in rds group but with name instance, which means it can collide with aws_rds_instance. Another example is aws_vpc_peering_connection where we use ec2 as group, hence the default kind name is calculated as PeeringConnection under ec2 group, which is less meaningful to users because it should've been VPCPeeringConnection. One example I could think of in this PR is azurerm_key_vault_key where we override the group as keyvault, however, if we don't manually override the kind it will be calculated as VaultKey, which is actually wrong so we need to make it omit 2 words and calculate the kind as Key.
We used that ReplaceGroupWords function as a solution so that we don't have to write all those trivial kind overrides but you can use some other function as well. But it's not really about what group to use, more about the final calculated kind.
There was a problem hiding this comment.
Thank you @muvaf for the clarification, I think I had taken it in the broader context. I've implemented a mechanism that defaults to dropping the first (_-separated) token and constructing the Kind name from the rest, and that allows overriding the number of initial tokens to be dropped for the entire group. And for cases where I would like to override only a specific Kind name (and not all the Kind names in the entire group), I have provided custom configurations under the config/<group> folder. The logic is the defaults affecting multiple resources are specified in a common place, but any specific overrides reside under config/<group> together with any other future configuration overrides like references, etc.
I think this logical organization makes sense but I can also copy the implementation in jet aws & gcp and maybe move those specific overrides to a common place like it's done there and remove single-resource kind-only configuration overrides. It looks more like our initial approach of putting all configuration into a single package.
There was a problem hiding this comment.
You can use the same implementation but it seems like the one you added does the job pretty well already, so all should be fine. Thank you!
- Use Microsoft provider name scraped from the Terraform registry as API Group names Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
There was a problem hiding this comment.
Could you print the whole list and add it to the PR description so that we can examine one by one? Similar to this one crossplane-contrib/provider-jet-gcp#25 (comment)
The following is what needs to be done to make it collapsible:
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
|
Hi @muvaf, |
There was a problem hiding this comment.
azurestackhci:
StackHCICluster -> azurerm_stack_hci_cluster -> Cluster?
cognitiveservices:
CognitiveAccount -> azurerm_cognitive_account -> Account?
CognitiveAccountCustomerManagedKey -> azurerm_cognitive_account_customer_managed_key -> AccountCustomerManagedKey?
datashare:
Share -> azurerm_data_share -> DataShare? // "Manages a Data Share." from TF docs.
eventhub:
Hub -> azurerm_eventhub -> EventHub? // "Manages a Event Hubs as a nested resource..." from TF docs.
healthbot:
Bot -> azurerm_healthbot -> HealthBot? // "Manages a Healthbot Service." from TF docs.
logic:
ActionCustom -> azurerm_logic_app_action_custom -> AppActionCustom? (goes for all other resources in logic group)
maintenance:
DedicatedHost -> azurerm_maintenance_assignment_dedicated_host -> MaintenanceDedicatedHost?
VirtualMachine -> azurerm_maintenance_assignment_virtual_machine -> MaintenanceVirtualMachine
VirtualMachineScaleSet -> azurerm_maintenance_assignment_virtual_machine_scale_set -> MaintenanceVirtualMachineScaleSet
notificationhubs:
NotificationHubAuthorizationRule -> azurerm_notification_hub_authorization_rule -> AuthorizationRule?
NotificationHubNamespace -> azurerm_notification_hub_namespace -> Namespace?
operationalinsights:
Cluster -> azurerm_log_analytics_cluster -> LogAnalyticsCluster (and goes for all others in operationalinsights group)
security:
Assessment -> azurerm_security_center_assessment -> SecurityCenterAssessment? (goes for others too since center is not included in group)
storagesync:
Sync -> azurerm_storage_sync -> StorageSync // "Manages a Storage Sync." from TF docs.
I have some suggestions above, some are more critical than others, though feel free to take whatever you feel suits more since it's hard to have strong opinions about this. LGTM after adresssing! Thanks!
Thank you @muvaf for reviewing the mappings. Following your suggestion, I opted for more explicit names ignoring using package names as prefixes. Following are the changes I've made: |
- Rename certain kinds Signed-off-by: Alper Rifat Ulucinar <[email protected]>
ulucinar
changed the title
ulucinar
changed the title
ulucinar
changed the title
Description of your changes
Fixes #108
API Groups for azurerm resources have been scraped from the
terraform importdocumentation from Terraform registry.I have:
make reviewable testto ensure this PR is ready for review.Click to see current resource grouping
How has this code been tested
Compiles successfully.