-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New Resource: Autoscale Settings #1140
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @YuriyKischenko for your valuable contribution. I have commented on your code changes, please take a look.
azurerm/config.go
Outdated
@@ -469,6 +472,12 @@ func (c *ArmClient) registerCosmosDBClients(endpoint, subscriptionId string, aut | |||
} | |||
|
|||
func (c *ArmClient) registerComputeClients(endpoint, subscriptionId string, auth autorest.Authorizer, sender autorest.Sender) { | |||
autoscaleSettingsClient := insights.NewAutoscaleSettingsClientWithBaseURI(endpoint, subscriptionId) | |||
setUserAgent(&autoscaleSettingsClient.Client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to use c.configureClient(&autoscaleSettingsClient.Client, auth)
here
azurerm/config.go
Outdated
@@ -469,6 +472,12 @@ func (c *ArmClient) registerCosmosDBClients(endpoint, subscriptionId string, aut | |||
} | |||
|
|||
func (c *ArmClient) registerComputeClients(endpoint, subscriptionId string, auth autorest.Authorizer, sender autorest.Sender) { | |||
autoscaleSettingsClient := insights.NewAutoscaleSettingsClientWithBaseURI(endpoint, subscriptionId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a monitoring related API, why not put it into the registerMonitorClients
?
"sort" | ||
"strconv" | ||
"time" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these empty lines by running make fmt
before commit to the repository.
Required: true, | ||
ForceNew: true, | ||
}, | ||
"resource_group_name": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function resourceGroupNameSchema()
could be used here.
"minimum": { | ||
Type: schema.TypeInt, | ||
Required: true, | ||
ValidateFunc: validation.IntBetween(1, 100), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do (striked out by @katbyte)validation.IntAtLeast(1)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of this range the azure API will return a junk error.
return err | ||
} | ||
|
||
d.SetId(*result.ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response ID
of CreateOrUpdate
may not be correct (especially the casing), so we are still required to use the Get
method to read the correct ID
just like other resources.
} | ||
|
||
func resourceArmAutoscaleSettingCreateOrUpdate(d *schema.ResourceData, meta interface{}) error { | ||
armClient := meta.(*ArmClient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
armClient
is not used anywhere else, we may change this to asClient := meta.(*ArmClient).autoscaleSettingsClient
.
autoscaleSetting := *result.AutoscaleSetting | ||
d.Set("name", result.Name) | ||
d.Set("resource_group_name", resGroup) | ||
d.Set("location", result.Location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set the location in a standard way like the one in redis cache.
|
||
func resourceArmAutoscaleSettingDelete(d *schema.ResourceData, meta interface{}) error { | ||
armClient := meta.(*ArmClient) | ||
asClient := armClient.autoscaleSettingsClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify the two lines above to: asClient := meta.(*ArmClient).autoscaleSettingsClient
recurrence, recurrenceErr := expandAzureRmAutoscaleRecurrence(profile) | ||
|
||
if fixedDate != nil && recurrence != nil { | ||
return nil, fmt.Errorf("Conflict between fixed_date and reucrrence in profile %s", profileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic could be simplified to the schema definition. Take storage blob as an example.
This comment has been minimized.
This comment has been minimized.
Merged requested changes except update of expandAzureRmAutoscaleProfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @YuriyKischenko for the update. Basically the code looks good to me, and just some small changes need to be made (for example the test location).
"Friday", | ||
"Saturday", | ||
"Sunday", | ||
}, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it case sensitive? If not, we may add DiffSuppressFunc: ignoreCaseDiffSuppressFunc
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this needs to be a case insensitive check.
|
||
read, err := asClient.Get(ctx, resourceGroupName, name) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor It would be better to be consistent with other error message (for example, including resource group name in the message).
"strings" | ||
"time" | ||
|
||
"github.com/Azure/azure-sdk-for-go/services/monitor/mgmt/2017-05-01-preview/insights" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion @tombuildsstuff @katbyte This package seems to be deprecated by Azure Go SDK team. Can we use the new one instead?
|
||
func listTimeZoneNames() []string { | ||
return []string{ | ||
"Dateline Standard Time", "UTC-11", "Hawaiian Standard Time", "Alaskan Standard Time", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please provide the source of this list?
|
||
resource "azurerm_resource_group" "test" { | ||
name = "acctas%d" | ||
location = "West US" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we don't hard code location in test cases (because we will pass it as an environment variable). Take this as an example.
Hi @YuriyKischenko , as you have not responded for more than two weeks, I would like to commit directly to your repository. |
Hi @JunyiYi |
Thanks @YuriyKischenko . I will just push code into your branch. So please just make sure the branch exists, and keep this PR open. |
Added @JunyiYi as a collaborator |
I'm going to contribute to the repository myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @YuriyKischenko and the updates @JunyiYi. I took a look at it and left some comments inline.
}, | ||
}, | ||
"fixed_date": { | ||
Type: schema.TypeSet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a List instead of a Set?
Set: resourceAzureRmAutoscaleDefaultHash, | ||
}, | ||
"recurrence": { | ||
Type: schema.TypeSet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, Shouldn't this be a List instead of a Set?
"Friday", | ||
"Saturday", | ||
"Sunday", | ||
}, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this needs to be a case insensitive check.
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"email": { | ||
Type: schema.TypeSet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a List instead of a Set?
Set: resourceAzureRmAutoscaleDefaultHash, | ||
}, | ||
"webhook": { | ||
Type: schema.TypeSet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, Shouldn't this be a List instead of a Set?
schedule["time_zone"] = *recurrence.Schedule.TimeZone | ||
|
||
days := make([]interface{}, len(*recurrence.Schedule.Days)) | ||
for k, v := range *recurrence.Schedule.Days { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to i, v and add nil checking?
schedule["hours"] = hours | ||
|
||
minutes := make([]interface{}, len(*recurrence.Schedule.Minutes)) | ||
for k, v := range *recurrence.Schedule.Minutes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here... Can we change this to i, v and add nil checking?
schedule["days"] = days | ||
|
||
hours := make([]interface{}, len(*recurrence.Schedule.Hours)) | ||
for k, v := range *recurrence.Schedule.Hours { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here... Can we change this to i, v and add nil checking?
|
||
# azurerm\_autoscale\_setting | ||
|
||
Create an [autoscale setting](https://docs.microsoft.com/en-us/azure/monitoring-and-diagnostics/monitoring-overview-autoscale) that can be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this from Create an
to Manage a
here?
|
||
* `default` - (Required) Specifies the number of instances that are available for scaling if metrics are not available for | ||
evaluation. The default is only used if the current instance count is lower than the default. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a HR between this section and all of the following sections as well?
---
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I accidentally clicked the wrong radio button on my previous review.
@JunyiYi we're looking to ship this as part of v1.10 of the Provider this week - as such I'm going to pick this up (I hope you don't mind!). Looking at the changes made I'm going to have to revert a couple of commits you've made (in particular the timezone refactoring [since this could potentially break the VM resource] and the moving to a deprecated SDK [Preview services not in the Preview folder are deprecated]) which will allow us to fix the remaining issues / allow us to ship this. Thanks! |
…quency` since it needs defaulting
a74bbcd
to
81b72f1
Compare
Tests pass: ``` $ acctests azurerm TestAccAzureRMAutoScaleSetting_multipleProfiles === RUN TestAccAzureRMAutoScaleSetting_multipleProfiles --- PASS: TestAccAzureRMAutoScaleSetting_multipleProfiles (410.92s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 411.254s ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this now LGTM with the refactoring 👍
@YuriyKischenko thanks for this PR - I hope you don't mind us finishing it off, but this'll ship as a part of v1.10 :) |
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
Updated existing closed PR #135 by @whiskeyjay to build with latest terraform azurerm provider code
(Fixes #50)