New Resource: azurerm_workloads_sap_virtual_instance
#22869
Conversation
|
@catriona-m , thanks for the comments. I updated PR. Please take another look. Thanks. |
There was a problem hiding this comment.
Thanks for pushing those changes.
I've taken a look through this and it appears that this PR takes a different approach to other parts of the provider - and needs to be made consistent.
Whilst I've left some comments inline, I've not commented on every instance of this issue due to the number of repeated examples - as such would you mind working through the comments, and applying these to the entire resource?
Alongside from the comments, there's really two major issues with this PR:
- Design issues have been dismissed but marked as resolved - so the design issue is still there. Whilst I can understand the desire to mark a comment as resolved, given that design issues are fundamental to the resource - would you mind not marking these as resolved so the team can work through these?
- Fundamental differences in approaches to the rest of the provider - such as returning
(object, error)rather than(*object, error). Whilst minor, these conventions exist so that we don't send invalid data to the API in the event of an error going unchecked (and instead panic) - but also apply for naming consistency across the provider (e.g.virtual_machine_namerather than vm_name`).
In addition, given the complexity of this resource, have we validated that the SKUs being used in the acceptance tests are the minimum we can get away with?
Once those comments have been addressed we can take another look through here - but as it stands we need to ensure this is brought into line with the rest of the provider.
Thanks!
| return result | ||
| } | ||
|
|
||
| func expandVirtualMachineFullResourceNames(input []VirtualMachineFullResourceNames) (sapvirtualinstances.SingleServerFullResourceNames, error) { |
There was a problem hiding this comment.
in literally every other part of the provider we return a pointer and an error - such that if there's an error we don't unintentionally use an invalid return object (so the provider instead panic's) - why is this file not returning a pointer and an error?
| return sapvirtualinstances.VirtualMachineConfiguration{} | ||
| } | ||
|
|
||
| virtualMachineConfiguration := &input[0] |
There was a problem hiding this comment.
... why are we making this a pointer?
| } | ||
| } | ||
|
|
||
| func SchemaForSAPVirtualInstanceLoadBalancerFullResourceNames() *pluginsdk.Schema { |
There was a problem hiding this comment.
these should be inlined and not separate functions - especially not public functions - can we inline these?
| ValidateFunc: validate.DiskName, | ||
| }, | ||
|
|
||
| "vm_name": { |
There was a problem hiding this comment.
we don't use vm but rather virtual_machine - can we update this to be virtual_machine_name?
| }, false), | ||
| }, | ||
|
|
||
| "count": { |
There was a problem hiding this comment.
so let's call this number_of_disks - count has other meanings in Terraform
Also: why was this issue marked as resolved? I'm concerned that this point was brushed over? cc @mybayern1974
| }) | ||
| } | ||
|
|
||
| func sapVirtualInstanceStateRefreshFunc(ctx context.Context, client *sapvirtualinstances.SAPVirtualInstancesClient, id sapvirtualinstances.SapVirtualInstanceId) pluginsdk.StateRefreshFunc { |
There was a problem hiding this comment.
(as above) this can be removed by simply adding the missing provisioningState into this list - can we make that change?
There was a problem hiding this comment.
The PR is submitted for this.
There was a problem hiding this comment.
Updated to use createThenPoll
| MaxItems: 1, | ||
| Elem: &pluginsdk.Resource{ | ||
| Schema: map[string]*pluginsdk.Schema{ | ||
| "availability_set_name": { |
There was a problem hiding this comment.
Service API would create it for the user
As you're describing above - this creates a series of issues where resources become untracked - and so instead of making this field optional, we should make this field Required so that users pass in an existing Availability Set ID, which can then be managed by TF - rather than introducing this workaround.
Users should never need to set the prevent_deletion_if_contains_resources flag in their configurations unless they want to explicitly manage resources outside of Terraform (e.g. an ARM Template). Setting this flag in acceptance tests means we're not designing the resource correctly - so we need to ensure we're setting users up for success (by which the default terraform apply and terraform destroy should succeed - not fail because the API is creating something users may not be aware of and not cleaning it up).
As such: we need to make this field Required rather than Optional in order to be able to ship this - so can we update that?
An side - why was this comment marked as resolved? This is a fundamental design issue with this resource /cc @mybayern1974
|
|
||
| os_profile { | ||
| admin_username = "testAdmin" | ||
| ssh_private_key = "-----BEGIN RSA PRIVATE KEY-----\nMIIG5AIBAAKCAYEAvJNStJo6QbcgUXK/u+Kes0oatPYTF5kGSSXpuNUZaldd9pGx\nlMvxB3EC6Dpqdqnb+is/44M+PWFjNlscYQfBvlIfBufH3mBWhjZE/lk63xP1yx8R\nZ1zIIWYAhIlfL3zVETrh7se1H7MYg7ejcNtteX5CfJUI0BHbij30uzpqEEA1Lxno\nPK8VG8KLmHUfc+TJnDSkogQtGdxBAVlZGNI7GwEmqxPYkSw0+Sa13nmVgknvv5YN\nzn3u29vH/p16PSx/76EVXPnirMek+q3lvcFbZusoBAV2W6r7hHqiEoC70hVlw+0r\nDtgm8iaZmjpM4yDG85Wh1dduvj2HQGNr39IFYQsEbecFP7nhZaDJk29x2y5MlXM5\nbgVLEn3Cdx+Q2DxAogsuaimj7Bhw8xRgcnP9GMvnzZ9i/1qzYDbgty2nrM02e2Kj\nVaP+rV0xkqjjK7/AA9az+9bF9hw0nZS3/x8i0YDY3yZ/ykd2RPUGdh5fU0XGfQzf\nlf8L3P5XIv+57EsdAgMBAAECggGAFkYchcKV0P9NbPFt3kZ1Ul4Va3yJYscra+Zz\nheZ92wa4zZAF9rpkHOnnWwDTZHLJzfHf2QK+jkd7jYcTgg6FfvJ6QbmM7SJZ9f5h\nBd4KSyEzbiucRaY66V7//qevO4+2JxPabfbe2QCxi5VcU89HTgtw1QBRiyog0WJi\nDt9mecbrwUWBHfHcP2wqSvbCoVDL04yQSabOoPhYIU2pbXofiyAGrjxo3zTmiOte\nngmkdEBBdlLGDLbpSMTcCaIWNzWTLvZVyNUgult1o1lmYloQ+/I9dISj//5PP3ii\nsG6dEN+qk/ALRxrzD1jP+M+KZTgkF7x2VtDEdbFXBYPbUkrawsKvoXw+nY9YaZeB\nrvcCpO7SAOasXMosPTwpZHkOHZiW//YHGQBO3QlKoN3DcgFhL+IeHG6kly7Lzr8B\nKimXkXKim/Fd77SpvJhMCiSPkZJiidrlOQjCjV3PPuxGOoZJHLHJddIXxFSV3mqP\nyobtadqS5Qdp0HR7JYlRMLveYNTtAoHBAOtxOFm1UtCKk5osVMjVYUjXw/isNnpF\n5hfHd68HeSinEx1idTvmNLtAC5hSddTvwtaTRKe88TA7Phb3QSL7n3TGKImbbmjy\nGtpFcQ3FUAsaWQNj0xcC10kpuWqit/t0PoFcUUAM6rIX8MhW4re2RoU4pWPNulI0\nA/PMNaQPhTXdDw7L5qqBjJnDUv3oenelQHVOGZRMA/yFXv6ZWiMBnPydp/1hOYmi\ne2Gp8ZHMKla96btyw/oBUTJnZ3X/NmKBewKBwQDNCoB5utZRXl7Exm2cxgirDG7E\naK0odBb8dj5+SLI55HgqcK0wCBChMaXMNwmYaLrVMcjqRaN4t9HyZZ/V/5o4anr9\nM5wSE85Ra3EtYEPgoYdwTkIlL/1YzwEfuFJgJc9hCaQVZYQ8aTalSYCD2Xx2bg4c\nRsLoPFBT0XznCuV7IaA2UhYW02zXxm1/d6FIdcUHwZ1IsArCYd46bgz5w0B7qokp\nFfKJY2TyB1AeVhx9ArposqbGaTjUkvGXmnQ8hkcCgcEAtTwiNGvvo7gIhtU5Lp+S\nk5ADuphWFylXRVa2OnV2PmTdwfDYbZN3Y+yZAFf5fEBTqvkSEEzRHF9+HA+YhGVN\nCYbADa0oAIDdSsfJjuAkDWfqvUFKbJwzPI5xvDQli9qfgtSddsB6qTzkjFLVkrUs\n87/3ECx9EGoZ4MGBSRjpYd0YijtLBFVU9cf1Sp56Jz99rs6/wfgB2ZCQ30sMp4XG\nYm65scH1mI0KjNNUsPaIYN0v3qspUHlTF4mhiqM6KfmhAoHBAK/lC3PiCQsClu/d\nfZjY9gSuhLNvTOSAOlvXoCK7gFFTopZd1OR4drOhoKbArDWX2ncb30zB8suTfcKg\n1W5CeG1fQyTFSmTjosGMFyojA/fG+iYorGu0cHToGAG7IMekh/Opzp4gWUFtzNgc\nZug1AaWjIe218mxBmXNeKfUWDukDXqpa3uIz+5JbggGwgaZkiWLvAFuj0YcRaA/d\n6rm0ezPbhxC86DReFPHfviZYHtZLKdi5MYLSL1OEv0Yb1Q067wKBwFZqsKIq3ORH\nd5Mo0pYCtiPriHvPCOYn6EuveD4K704HWEwY5ALTvzzNu46IRFLMcrHOY+b20Oxx\n6HAE49M/BQiB9xgYVtf6ewRryDVW18jaa9nQL164ouaE5XNfCbyAHz/1tRFtFYlt\nVBHphNuxv8XtdVUj1tDGVwssYuSHThl8qOzNoKD3ZWSEBnzYea+5kW0djMqEI2PO\nefkhFBgGMcFl6oMA0ZYZqEEwsIouCIrnSYVfVNBFtqT6eoiBFhC4Ig==\n-----END RSA PRIVATE KEY-----\n" |
There was a problem hiding this comment.
We cannot ship a hardcoded ssh private key in an acceptance test - that's a security issue waiting to happen.
Terraform resources exist to provision an SSH key pair, so we should be using that instead
| secondary_ip_enabled = true | ||
|
|
||
| virtual_machine_configuration { | ||
| vm_size = "Standard_E32ds_v4" |
There was a problem hiding this comment.
this is huge (and thus, incredibly expensive to run) - is this the minimum we can get away with here?
There was a problem hiding this comment.
Yes. I have set the minimum values in acctest.
| %s | ||
|
|
||
| resource "azurerm_workloads_sap_virtual_instance" "test" { | ||
| name = "X%d" |
There was a problem hiding this comment.
this should be acctest as in every other resource
There was a problem hiding this comment.
Service API indicates that it should start with "X" and two numbers.
…asedDiscoveryConfiguration
|
@tombuildsstuff , thanks for the comments. I updated PR. Please take another look. Thanks. |
|
hey @neil-yechenwei Apologies for the delayed reply to this one. Having chatted about this PR internally, we’ve come to the conclusion that since this resource is provisioning substantially different types of cluster (e.g. products) in substantially different configurations (e.g. configure an existing cluster, a single server deployment, and a full three-tier cluster) that this resource needs to be split. This mirrors the approach that we’ve taken in other resources, for example HDInsight where we split this by the Cluster Kind (e.g. Hadoop/HBase) which would map to the different Products being deployed here. Given the number of different configurations possible here, unfortunately a single resource is going to become unmanageable (for provider developers) and extremely hard to use (for users) in addition to being hard to test/easy to introduce regressions to, due to the levels of validation that are going to be required to cover all of the different scenarios. Instead breaking this resource apart allows us to provide a granular validation (as we do with HDInsight) to both make this resource more usable for users and allows better representation of the resource, meaning this is easier to test - and thus less likely to introduce regressions. As such whilst I’d like to thank you for this contribution, unfortunately we’re going to need to split this PR into different sub-resources to continue - one thing I think we need to clarify is whether there’s substantially different behaviours for the products which can be deployed on a Cluster or not to determine whether we need to split this by product and by cluster type - or just by cluster-type. However since this PR requires splitting - rather than leaving this PR open and trying to discuss each of the configurations here, I’m going to suggest that we close this PR for the moment and open dedicated issues tracking each of the cluster configurations (e.g. Thanks! |
|
@tombuildsstuff , thanks for the suggestion. I will close this PR. As for TF resource design, I suggest to split this resource per the parent configuration type ("DeploymentWithOSConfiguration/DiscoveryConfiguration") not the sub configuration type("SingleServer/ThreeTier") since the sub configuration type only belongs to the parent configuration type "DeploymentWithOSConfiguration". Service team also agrees my this suggestion. The parent configuration type "Deployment With OS Configuration" means that it would create SAP Virtual Instance based on new SAP System. If we split it per the sub configuration type "SingleServer/ThreeTier", then we still need to design a new resource like azurerm_workloads_sap_virtual_instance_discovery_configuration per the parent configuration type to support "DiscoveryConfiguration". So I assume we should split the resource per one kind of configuration type not mixed configuration types. Does it make sense? Current TF resource design in this PR: New TF resource design I suggested: |
|
Seems it makes sense to split the resource per sub configuration type SingleServer/ThreeTier. So I will split it per sub configuration type for the parent configuration type DeploymentWithOSConfiguration. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
--- PASS: TestAccWorkloadsSAPVirtualInstance_basic (2243.90s)
--- PASS: TestAccWorkloadsSAPVirtualInstance_requiresImport (2297.98s)
--- PASS: TestAccWorkloadsSAPVirtualInstance_complete (2504.55s)
--- PASS: TestAccWorkloadsSAPVirtualInstance_update (2321.62s)
--- PASS: TestAccWorkloadsSAPVirtualInstance_transportMount (2814.86s)
--- PASS: TestAccWorkloadsSAPVirtualInstance_discoveryConfiguration (918.50s)
Note: Before using this resource, it's required to submit the request of registering the Resource Provider with Azure CLI
az provider register --namespace "Microsoft.Workloads". The Resource Provider can take a while to register, you can check the status by runningaz provider show --namespace "Microsoft.Workloads" --query "registrationState". Once this outputs "Registered" the Resource Provider is available for use. I also added this note in the resource doc.