Add alicloud provider, including resource and examples of alicloud provider #10761
Conversation
There was a problem hiding this comment.
Hi @xiaozhu36
thanks for the work here - I have left a lot of comments - mostly stylistic inline
The most comment is that we are using d.SetPartial in Create funcs and we don't need to
The second is that every time we have a d.HasChange in the Updatefunc, we have an update call - can these not be batched up? I.e. if a security group has a name and a description change, does it need to be 2 update commands or just 1?
Apart from that, it is looking good. You will need to govendor the github.com/denverdino/aliyungo/common as that is causing build errors
Thanks
Paul
| //Modify with your Access Key Id and Access Key Secret | ||
|
|
||
| const ( | ||
| TestAccessKeyId = "****" |
There was a problem hiding this comment.
I don't think we should hard code these - we should use environment variables to pass these in - this is a standard practice across providers
| TestInstanceId = "MY_TEST_INSTANCE_ID" | ||
| TestSecurityGroupId = "MY_TEST_SECURITY_GROUP_ID" | ||
| TestImageId = "MY_TEST_IMAGE_ID" | ||
| TestAccountId = "MY_TEST_ACCOUNT_ID" //Get from https://account.console.aliyun.com |
There was a problem hiding this comment.
Again this should not be hard coded
| } | ||
|
|
||
| func testAccPreCheck(t *testing.T) { | ||
| if v := os.Getenv("ALICLOUD_PROFILE"); v == "" { |
There was a problem hiding this comment.
what is ALICLOUD_PROFILE? That's not mentioned in the provider.go
| d.SetId(diskID) | ||
| d.Partial(true) | ||
|
|
||
| d.SetPartial("name") |
There was a problem hiding this comment.
We don't need to SetPartial here for any values
|
|
||
| log.Printf("[DEBUG] DescribeDiskAttribute for instance: %#v", disks) | ||
|
|
||
| if disks != nil && len(disks) > 0 { |
There was a problem hiding this comment.
I would handle this slightly differenly -
if disks == nil || len(disks) == 0 {
return fmt.Errorf("No disks found")
}
That way you don't have to wrap the d.Set in the if statement
| }, | ||
|
|
||
| "bandwidth_packages": &schema.Schema{ | ||
| Type: schema.TypeList, |
There was a problem hiding this comment.
As I see only 4 bandwidth_packages are allowed, we can add:
MaxItems: 4 to the schema
We can then remove the if len() > 4 check below
| if v, ok := d.GetOk("name"); ok { | ||
| name = v.(string) | ||
| } | ||
| // } else { |
There was a problem hiding this comment.
Can this be removed or is it a todo?
| d.SetPartial("spec") | ||
| d.SetPartial("vpc_id") | ||
|
|
||
| // for i, packageId := range resp.BandwidthPackageIds.BandwidthPackageId { |
| } | ||
|
|
||
| d.SetId(resp.NatGatewayId) | ||
| d.Partial(true) |
There was a problem hiding this comment.
We don't need d.Partial in Create
| spec = NatGatewaySmallSpec | ||
| } | ||
|
|
||
| args := &ModifyNatGatewaySpecArgs{ |
There was a problem hiding this comment.
Follow the pattern of a single conn.ModifyNatGatewaySpec and pass the args in
|
Hi @stack72 Thanks very much for your help and review advise, and our team have improve existing defects in my provider as per your advise. Now, we want to pull a new request and the PR has the following changes:
Please review them and looking forward to a good news. Thanks very much. Thanks Guimin He |
|
Closed via #11235 |
|
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
This is terraform alicloud provider provided by Official AliCloud. The provider is aim at building, changing, and managing alicloud infrastructure safely and efficiently based on terraform powerful management capabilities for infrastructure. At present, it has supported several alicloud resource, such as ECS, VPC, SLB, Security Group and EIP. With the continuous development and improvement to the provider, it will have more perfect function and support more alicloud resource. We hope the PR to integrate alicloud provider into terraform and help consumer to manage alicloud infrastructure. If you have any question, we are glad to get feedback as soon as possible, thanks a lot.