resource/alicloud_ecs_disk: Renames alicloud_disk to alicloud_ecs_disk; Deprecates the name and use disk_name instead; Adds more attribute, like payment_type, encrypt_algorithm, auto_snapshot and so on; Upgrades the its dependence sdk #3443

lixue323 · 2021-04-14T09:10:13Z

No description provided.

zql-tqy · 2021-04-14T09:49:48Z

alicloud/data_source_alicloud_ecs_disks.go

+				Type:         schema.TypeString,
+				Optional:     true,
+				ForceNew:     true,
+				ValidateFunc: validation.StringInSlice([]string{"PostPaid", "PrePaid"}, false),


这个需要录入一个出入参映射的转换使用 "PayAsYouGo", "Subscription" 这两个枚举值

zql-tqy · 2021-04-15T02:12:58Z

alicloud/resource_alicloud_ecs_disk.go

+			"auto_pay": {
+				Type:     schema.TypeBool,
+				Optional: true,
+				Default:  true,


这个属性没有在文档中说明
另外这个属性应该设置默认值吧，如果是后付费不应该设置这个属性吧，需要添加个压制函数，自动化代码中应该可以通配上

这个参数，产品说api支持了，但是还没对外开放

zql-tqy · 2021-04-15T03:14:14Z

alicloud/service_alicloud_ecs.go

+		return object, WrapErrorf(err, FailedGetAttributeMsg, id, "$.Disks.Disk", response)
+	}
+	if len(v.([]interface{})) < 1 {
+		return object, WrapErrorf(Error(GetNotFoundMessage("ECS", id)), NotFoundWithResponse, response)


这块自动化的应该有点问题，应该跟上面一样抛出 GetNotFoundMessage("EcsDisk", id) 调整下吧
下面的也是

zql-tqy · 2021-04-15T03:16:07Z

website/docs/d/ecs_disks.html.markdown

+		* `attached_time` - A mount of time.
+		* `device` - The mount point of the disk.
+		* `instance_id` - The instance ID of the disk mount.
+	* `` - The name.


这个检查下

zql-tqy · 2021-04-15T03:17:08Z

website/docs/r/disk.html.markdown

@@ -11,6 +11,8 @@ description: |-

 Provides a ECS disk resource.

+-> **DEPRECATED:** This resource has been renamed to [alicloud_ecs_key_pair](https://www.terraform.io/docs/providers/alicloud/r/ecs_key_pair.html) from version 1.122.0.


这个替代资源放错了吧

zql-tqy · 2021-04-15T03:18:43Z

website/docs/r/ecs_disk.html.markdown

+    * You cannot specify both the `availability_zone` and `instance_id` parameters.
+* `kms_key_id` - (Optional, ForceNew) The ID of the KMS key corresponding to the data disk, The specified parameter `Encrypted` must be `true` when KmsKeyId is not empty.
+* `name` - (Optional, Computed, Deprecated in v1.122.0+) Field `name` has been deprecated from provider version 1.122.0. New field `disk_name` instead.
+* `payment_type` - (Optional) Payment method for disk. Valid values: `PostPaid`, `PrePaid`.


这里的枚举值记得修改

zql-tqy · 2021-04-15T03:20:00Z

website/docs/r/ecs_disk.html.markdown

+* `delete_auto_snapshot` - (Optional) Indicates whether the automatic snapshot is deleted when the disk is released. Default value: `false`.
+* `delete_with_instance` - (Optional) Indicates whether the disk is released together with the instance. Default value: `false`.
+* `description` - (Optional) Description of the disk. This description can have a string of 2 to 256 characters, It cannot begin with http:// or https://. Default value is null.
+* `disk_name` - (Optional, Computed) Name of the ECS disk. This name can have a string of 2 to 128 characters, must contain only alphanumeric characters or hyphens, such as "-",".","_", and must not begin or end with a hyphen, and must not begin with http:// or https://. Default value is null.


such as "-", ".", "_", and must not begin or end with a hyphen, and must not begin with `http://` or `https://`. Default value is `null`.

liuri1205 · 2021-04-16T06:12:53Z

alicloud/data_source_alicloud_ecs_disks.go

+	}
+	request["PageSize"] = PageSizeLarge
+	request["PageNumber"] = 1
+	request["MaxResults"] = PageSizeLarge


这个接口同时支持 Page 和 MaxResults 查询, 如果使用了 Page 分页, MaxResults 参数需要过滤掉

liuri1205 · 2021-04-16T06:15:07Z

alicloud/data_source_alicloud_ecs_disks.go

+			break
+		}
+		request["PageNumber"] = request["PageNumber"].(int) + 1
+		if nextToken, ok := response["NextToken"].(string); ok && nextToken != "" {


这里也是NextToken 不需要

liuri1205 · 2021-04-16T06:51:37Z

alicloud/resource_alicloud_ecs_disk_attachment.go

+		return WrapError(err)
+	}
+	wait := incrementalWait(3*time.Second, 3*time.Second)
+	err = resource.Retry(d.Timeout(schema.TimeoutCreate), func() *resource.RetryError {


d.Timeout(schema.TimeoutCreate) 这个需要在 schema 中定义, update 中也是一样

liuri1205 · 2021-04-16T06:53:58Z

website/docs/d/ecs_disks.html.markdown

+* `kms_key_id` - (Optional, ForceNew) The kms key id.
+* `name_regex` - (Optional, ForceNew) A regex string to filter results by Disk name.
+* `output_file` - (Optional) File name where to save data source results (after running `terraform plan`).
+* `payment_type` - (Optional, ForceNew) Payment method for disk.


payment_type 枚举值

liuri1205 · 2021-04-16T06:55:15Z

website/docs/r/ecs_disk_attachment.html.markdown

+
+Basic usage
+
+```


xiaozhu36 · 2021-04-25T02:07:52Z

alicloud/resource_alicloud_ecs_disk_test.go

+				ResourceName:            resourceId,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"advanced_features", "auto_pay", "dedicated_block_storage_cluster_id", "dry_run", "encrypt_algorithm", "storage_set_id", "storage_set_partition_number"},


Please check all of ImportStateVerifyIgnore and I think there are some attribute should not be here.

xiaozhu36 · 2021-04-25T02:09:22Z

alicloud/resource_alicloud_ecs_disk_attachment_test.go

+	"github.com/hashicorp/terraform-plugin-sdk/terraform"
+)
+
+func testAccCheckDiskAttachmentDestroy(s *terraform.State) error {


Is this method also used? I think using new test structure can instead.

xiaozhu36 · 2021-04-25T02:10:13Z

alicloud/resource_alicloud_ecs_disk_attachment.go

+			},
+			"delete_with_instance": {
+				Type:     schema.TypeBool,
+				Optional: true,


I think the bootable and delete_with_instance should have default value.

xiaozhu36 · 2021-04-25T02:11:58Z

alicloud/resource_alicloud_ecs_disk_attachment.go

+
+	if v, ok := d.GetOk("device"); ok {
+		request["Device"] = v
+	}


I think the block 83-85 can be removed because of it has been useless.

xiaozhu36 · 2021-04-25T02:12:54Z

alicloud/resource_alicloud_ecs_disk_attachment.go

+	d.Set("disk_id", disk["DiskId"])
+
+	if strings.HasPrefix(disk["Device"].(string), "/dev/x") {
+		disk["Device"] = "/dev/" + disk["Device"].(string)[len("/dev/x"):]


I think there is no need to do more joint jobs.

xiaozhu36 · 2021-04-27T02:39:09Z

alicloud/data_source_alicloud_ecs_disks.go

+		request["AutoSnapshotPolicyId"] = v
+	}
+	if v, ok := d.GetOk("availability_zone"); ok {
+		request["ZoneId"] = v


deprecated availability_zone and using zone_id instead

xiaozhu36 · 2021-04-27T02:41:25Z

alicloud/resource_alicloud_ecs_disk.go

+				Optional: true,
+				ForceNew: true,
+			},
+			"availability_zone": {


deprecated the. availability_zone and using zone_id instead.

xiaozhu36 · 2021-04-27T02:47:33Z

alicloud/resource_alicloud_ecs_disk.go

+			"DiskId": d.Id(),
+		}
+		request["NewSize"] = d.Get("size")
+		request["Type"] = "online"


expose the type to an attaibute.

xiaozhu36 · 2021-04-27T07:32:18Z

alicloud/data_source_alicloud_ecs_disks.go

+		request["DiskType"] = v
+	}
+	if v, ok := d.GetOk("type"); ok {
+		request["DiskType"] = v


Please check line 399-401.

新参数diskType, 老参数Type, 做兼容

xiaozhu36 · 2021-04-27T07:33:22Z

alicloud/data_source_alicloud_ecs_disks.go

+	}
+	if v, ok := d.GetOk("availability_zone"); ok {
+		request["ZoneId"] = v
+	}


If zone_id is specified, availability_zone should be ingored.

xiaozhu36 · 2021-04-27T07:37:59Z

alicloud/resource_alicloud_ecs_disk.go

+			"category": {
+				Type:         schema.TypeString,
+				Optional:     true,
+				ValidateFunc: validation.StringInSlice([]string{"cloud", "cloud_efficiency", "cloud_essd", "cloud_ssd"}, false),


The enum values are not equal to its datasource

xiaozhu36 · 2021-04-27T07:39:27Z

alicloud/resource_alicloud_ecs_disk.go

+			"type": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  "offline",


Does the type have enum values?

xiaozhu36 · 2021-04-27T07:42:40Z

alicloud/resource_alicloud_ecs_disk_attachment_test.go

+	available_resource_creation= "VSwitch"
+}
+resource "alicloud_ecs_disk" "default" {
+	availability_zone = "${data.alicloud_zones.disk.zones.0.id}"


It should be zone_id

xiaozhu36 · 2021-04-27T07:43:38Z

alicloud/resource_alicloud_ecs_disk_test.go

+				continue
+			}
+			sweeped = true
+			action = "DeleteDisk"


The line 76 should be action := "DeleteDisk"

前边已经定义了action，查询接口调用之后，去做删除

xiaozhu36 · 2021-04-27T07:43:53Z

alicloud/resource_alicloud_ecs_disk_test.go

+			{
+				Config: testAccConfig(map[string]interface{}{
+					"disk_name":         name,
+					"availability_zone": "${data.alicloud_zones.default.zones.0.id}",


xiaozhu36 · 2021-04-27T07:45:51Z

website/docs/d/ecs_disks.html.markdown

+
+* `additional_attributes` - (Optional, ForceNew) Other attribute values. Currently, only the incoming value of IOPS is supported, which means to query the IOPS upper limit of the current disk.
+* `auto_snapshot_policy_id` - (Optional, ForceNew) Query cloud disks based on the automatic snapshot policy ID.
+* `availability_zone` - (Optional, ForceNew) ID of the free zone to which the disk belongs.


Please keep sync the attribute with the schema.

xiaozhu36 · 2021-04-27T07:47:11Z

website/docs/d/ecs_disks.html.markdown

+    * `expiration_time` - Disk expiration time.
+    * `tags` - A map of tags assigned to the disk.
+    * `resource_group_id` - The Id of resource group.
+	* `auto_snapshot_policy_id` - Query cloud disks based on the automatic snapshot policy ID.


There should not have indent.

xiaozhu36 · 2021-04-27T07:47:41Z

website/docs/r/ecs_disk.html.markdown

+
+```terraform
+resource "alicloud_ecs_disk" "example" {
+  availability_zone = "cn-beijing-b"


Please use the latest attribute name.

xiaozhu36 · 2021-04-28T02:25:36Z

alicloud/service_alicloud_ecs.go

+	})
+	if err != nil {
+		if IsExpectedErrors(err, []string{"InvalidDiskChargeType.NotFound", "InvalidDiskIds.ValueNotSupported", "InvalidFilterKey.NotFound", "InvalidFilterValue", "InvalidLockReason.NotFound"}) {
+			return object, WrapErrorf(Error(GetNotFoundMessage("ECS:Disk", id)), NotFoundMsg, ProviderERROR, fmt.Sprint(response["RequestId"]))


Why so many error codes?

xiaozhu36 · 2021-04-28T02:26:19Z

alicloud/resource_alicloud_ecs_disk_test.go

+				log.Printf("[ERROR] Failed to delete Disk (%s): %s", item["DiskName"].(string), err)
+			}
+			if sweeped {
+				time.Sleep(5 * time.Second)


remove to outside of for block.

xiaozhu36 · 2021-04-28T02:26:46Z

alicloud/resource_alicloud_ecs_disk_attachment.go

+	}
+
+	ecsService := EcsService{client}
+	oldDisk, err := ecsService.DescribeEcsDisk(d.Get("disk_id").(string))


Why there is a describe action?

兼容之前代码，绑定前后，属性可能会改变，如果对比之后改变了，之后会调用更新方法改回来

xiaozhu36 · 2021-04-28T02:27:52Z

alicloud/data_source_alicloud_ecs_disks.go

+				Optional: true,
+				ForceNew: true,
+			},
+			"disk_type": {


I found a default value all in the docs.

xiaozhu36 · 2021-04-28T02:30:42Z

website/docs/r/ecs_disk.html.markdown

+ECS Disk can be imported using the id, e.g.
+
+```
+$ terraform import alicloud_ecs_disk.example <id>


using a fake resource id, not

zql-tqy suggested changes Apr 15, 2021

View reviewed changes

lixue323 force-pushed the change branch 5 times, most recently from a1fdfbb to 4d9cecb Compare April 16, 2021 05:24

liuri1205 suggested changes Apr 16, 2021

View reviewed changes

lixue323 force-pushed the change branch 7 times, most recently from cdd8867 to 8adc101 Compare April 21, 2021 10:59

xiaozhu36 requested changes Apr 25, 2021

View reviewed changes

lixue323 force-pushed the change branch 3 times, most recently from 864116a to ab8f9bf Compare April 27, 2021 02:21

xiaozhu36 requested changes Apr 27, 2021

View reviewed changes

lixue323 force-pushed the change branch from ab8f9bf to 386cf66 Compare April 27, 2021 04:59

xiaozhu36 requested changes Apr 27, 2021

View reviewed changes

lixue323 force-pushed the change branch 2 times, most recently from acae4ee to 266cdb6 Compare April 27, 2021 09:50

xiaozhu36 reviewed Apr 28, 2021

View reviewed changes

xiaozhu36 requested changes Apr 28, 2021

View reviewed changes

lixue323 force-pushed the change branch 2 times, most recently from 74091ed to f40e2f1 Compare April 29, 2021 03:34

disk

49600b0

lixue323 force-pushed the change branch from f40e2f1 to 49600b0 Compare April 29, 2021 08:17

xiaozhu36 merged commit 7e0de8b into aliyun:master Apr 29, 2021

		@@ -11,6 +11,8 @@ description: \|-

		Provides a ECS disk resource.

		-> DEPRECATED: This resource has been renamed to [alicloud_ecs_key_pair](https://www.terraform.io/docs/providers/alicloud/r/ecs_key_pair.html) from version 1.122.0.


		Basic usage

		```

resource/alicloud_ecs_disk: Renames alicloud_disk to alicloud_ecs_disk; Deprecates the name and use disk_name instead; Adds more attribute, like payment_type, encrypt_algorithm, auto_snapshot and so on; Upgrades the its dependence sdk #3443

resource/alicloud_ecs_disk: Renames alicloud_disk to alicloud_ecs_disk; Deprecates the name and use disk_name instead; Adds more attribute, like payment_type, encrypt_algorithm, auto_snapshot and so on; Upgrades the its dependence sdk #3443

Conversation

lixue323 commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment