Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/alicloud_ess_notification: add attribute of time_zone. #8027

Merged
merged 1 commit into from
Dec 26, 2024

Conversation

fuliu-zln
Copy link
Contributor

No description provided.

@fuliu-zln fuliu-zln force-pushed the feat/notify branch 3 times, most recently from 8997e09 to cbd1273 Compare December 25, 2024 07:14
@@ -35,17 +38,24 @@ func resourceAlicloudEssNotification() *schema.Resource {
Required: true,
ForceNew: true,
},
"time_zone": {
Type: schema.TypeString,
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个新增的属性,如果未指定会不会有返回值?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不会有返回值

),
},
{
ResourceName: resourceId,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

导入应该放在最后一步测试中

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

conn, err := client.NewEssClient()
request["ScalingGroupId"] = d.Get("scaling_group_id").(string)
request["NotificationArn"] = d.Get("notification_arn").(string)
request["TimeZone"] = d.Get("time_zone").(string)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time_zone不是必填的,对time_zone进行d.getOk判断在传值

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -55,18 +65,20 @@ func resourceAlicloudEssNotificationCreate(d *schema.ResourceData, meta interfac
}
}
if len(notificationTypes) > 0 {
request.NotificationType = &notificationTypes
request["NotificationType"] = &notificationTypes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

你写代码能不能不要直接复制粘贴?request["NotificationType"]为什么要赋值地址?request.NotificationType之所以传值带有&,是因为他是一个指针!!!
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.这块代码不是直接复制粘贴 ,这块代码是修改,因结构体的request没有TimeZone 才进行转为非结构体request的修改
2. request["NotificationType"] = &notificationTypes 这样写,测试过程中并没有出现问题,所以默认了这种写法
3. request["NotificationType"] = &notificationTypes 这样写,在Go结构体系的不规范,已进行了修改完善

return essClient.CreateNotificationConfiguration(request)
runtime := util.RuntimeOptions{}
runtime.SetAutoretry(true)
err = resource.Retry(d.Timeout(schema.TimeoutCreate), func() *resource.RetryError {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

你这么写的意义是什么?他重试了吗?有NeedRetry吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.这么写确实会重试,这个之前本地跑测试,重试行为是出现过的
2.加重试的原因,是伸缩组处于其他活动进行中/其他API操作时,该行为可能会失败,所以用了重试
3.如果是觉得缺少NeedRetry 不规范,这边增加了NeedRetry进行完善

}
addDebug(request.GetActionName(), raw, request.RpcRequest, request)
if d.HasChange("time_zone") {
request["TimeZone"] = d.Get("time_zone").(string)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time_zone不是必填的,不要直接传值,d.getOK判断

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if err != nil {
return WrapErrorf(err, DefaultErrorMsg, d.Id(), request.GetActionName(), AlibabaCloudSdkGoERROR)
if len(notificationTypes) > 0 {
request["NotificationType"] = &notificationTypes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不要复制粘贴

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
runtime := util.RuntimeOptions{}
runtime.SetAutoretry(true)
err = resource.Retry(d.Timeout(schema.TimeoutUpdate), func() *resource.RetryError {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么加重试?你这么写会重试吗?有NeedRetry吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.这么写确实会重试,这个之前本地跑测试,重试行为是出现过的
2.加重试的原因,是伸缩组处于其他活动进行中/其他API操作时,该行为可能会失败,所以用了重试
3.如果是觉得缺少NeedRetry 不规范,这边增加了NeedRetry进行完善


}

func TestAccAliCloudEssNotification_timeZone(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time_zone非必填,新增一个单测,第0步,也就是创建操作,不指定time_zone;只在更新操作指定

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -87,6 +87,7 @@ The following arguments are supported:
* account-id: the ID of your account.
* resource-relative-id: the notification method. Valid values : `cloudmonitor`, MNS queue: `queue/{queuename}`, Replace the queuename with the specific MNS queue name, MNS topic: `topic/{topicname}`, Replace the topicname with the specific MNS topic name.
* `notification_types` - (Required) The notification types of Auto Scaling events and resource changes. Supported notification types: 'AUTOSCALING:SCALE_OUT_SUCCESS', 'AUTOSCALING:SCALE_IN_SUCCESS', 'AUTOSCALING:SCALE_OUT_ERROR', 'AUTOSCALING:SCALE_IN_ERROR', 'AUTOSCALING:SCALE_REJECT', 'AUTOSCALING:SCALE_OUT_START', 'AUTOSCALING:SCALE_IN_START', 'AUTOSCALING:SCHEDULE_TASK_EXPIRING'.
* `time_zone` - (Optional, Available since v1.239.0) The time zone of the notification. Specify the value in UTC. For example, a value of UTC+8 specifies that the time is 8 hours ahead of Coordinated Universal Time, and a value of UTC-7 specifies that the time is 7 hours behind Coordinated Universal Time.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

240

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Collaborator

@MrWolong MrWolong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved

@ChenHanZhang ChenHanZhang merged commit 25f7256 into aliyun:master Dec 26, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants