new resource azurerm_mssql_managed_instance_start_stop_schedule
#26702
Conversation
github-actions
bot
added
documentation
dependencies
service/mssqlmanagedinstance
size/XL
waiting-response
and removed
waiting-response
labels
|
|
||
| func (r MsSqlManagedInstanceStartStopScheduleResource) Attributes() map[string]*pluginsdk.Schema { | ||
| return map[string]*pluginsdk.Schema{ | ||
| "name": { |
There was a problem hiding this comment.
It's not necessary to output these fields.
There was a problem hiding this comment.
Updated
| } | ||
|
|
||
| func (r MsSqlManagedInstanceStartStopScheduleResource) Arguments() map[string]*pluginsdk.Schema { | ||
| return map[string]*pluginsdk.Schema{ |
There was a problem hiding this comment.
Add a name argument with a validation func which only allows "default".
There was a problem hiding this comment.
I have confirmed with the service team that this resource will have 1to1 relationship with the managedinstance and hence the name will always be "default", I think leaving this out so that user doesn't need to declare it is cleaner.
| Func: func(ctx context.Context, metadata sdk.ResourceMetaData) error { | ||
| client := metadata.Client.MSSQLManagedInstance.ManagedInstanceStartStopSchedulesClient | ||
|
|
||
| managedInstanceId, err := commonids.ParseSqlManagedInstanceID(metadata.ResourceData.Get("managed_instance_id").(string)) |
There was a problem hiding this comment.
Instead of getting the managedInstanceId from the resource data, it's recommended to parse and build it like the following codes.
id, err := parse.ManagedInstanceStartStopScheduleID(metadata.ResourceData.Id())
if err != nil {
return err
}
managedInstanceID := commonids.NewSqlManagedInstanceID(id.SubscriptionId, id.ResourceGroup, id.ManagedInstanceName)
There was a problem hiding this comment.
Updated
| } | ||
|
|
||
| if model := resp.Model; model != nil { | ||
| if name := model.Name; name != nil { |
There was a problem hiding this comment.
Use the name from the parsed ID instead of from the response, because the upstream API might have unexpected changes.
There was a problem hiding this comment.
Updated
| } | ||
|
|
||
| if properties := model.Properties; properties != nil { | ||
| if properties.Description != nil { |
There was a problem hiding this comment.
It could be simplified to state.Description = pointer.From(properties.Description), please update other places.
There was a problem hiding this comment.
Updated
There was a problem hiding this comment.
Thanks for this PR @hqhqhqhqhqhqhqhqhqhqhq. We just merged a PR that migrates all the managed instance resources over to hashicorp/go-azure-sdk so this will want to be rebased on top of that.
Overall this is off to a good start, I've left a detailed review in-line, once the comments have been resolved we can take another look through.
| Optional: true, | ||
| }, | ||
|
|
||
| "schedule_list": { |
There was a problem hiding this comment.
_list is redundant here, can we rename this to
| "schedule_list": { | |
| "schedule": { |
There was a problem hiding this comment.
Updated
| }, | ||
| }, | ||
|
|
||
| "timezone_id": { |
There was a problem hiding this comment.
I think this should be shortened to
| "timezone_id": { | |
| "timezone": { |
There was a problem hiding this comment.
Thanks for this, the parent resource azurerm_mssql_managed_instance uses timezone_id, I think it would nice to have similar terminologies.
| managedInstanceId, err := commonids.ParseSqlManagedInstanceID(model.SqlManagedInstanceId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if managedInstanceId == nil { | ||
| return fmt.Errorf("managedInstanceId is nil") | ||
| } | ||
|
|
||
| id := *managedInstanceId |
There was a problem hiding this comment.
model.SqlManagedInstanceId is a required property in the schema with strict validation on it so we can expect the managedInstanceId to not ever be nil with a high degree of confidence, which is why we don't nil check for this in any other resources in the provider.
For consistency can we please update this to
| managedInstanceId, err := commonids.ParseSqlManagedInstanceID(model.SqlManagedInstanceId) | |
| if err != nil { | |
| return err | |
| } | |
| if managedInstanceId == nil { | |
| return fmt.Errorf("managedInstanceId is nil") | |
| } | |
| id := *managedInstanceId | |
| id, err := commonids.ParseSqlManagedInstanceID(model.SqlManagedInstanceId) | |
| if err != nil { | |
| return err | |
| } |
There was a problem hiding this comment.
Updated
| properties := resp.Model | ||
| if properties == nil { | ||
| return fmt.Errorf("retrieving %s: properties was nil", managedInstanceID) | ||
| } |
There was a problem hiding this comment.
In the update we usually nil check both Model and Properties to prevent the provider panicking in the unlikely event that these are nil (the SDK actually handles this for us).
For consistency can we please update this to
| properties := resp.Model | |
| if properties == nil { | |
| return fmt.Errorf("retrieving %s: properties was nil", managedInstanceID) | |
| } | |
| properties := resp.Model | |
| if resp.Model == nil { | |
| return fmt.Errorf("retrieving %s:`model` was nil", managedInstanceID) | |
| } | |
| if resp.Model.Properties == nil { | |
| return fmt.Errorf("retrieving %s:`properties` was nil", managedInstanceID) | |
| } | |
| payload := resp.Model |
There was a problem hiding this comment.
Updated
| } | ||
| } | ||
|
|
||
| properties.SystemData = nil |
There was a problem hiding this comment.
We shouldn't need to do this. If this is required then this is indicative of an API bug and an issue should be opened on the Rest API specs repo and linked in a comment above this.
There was a problem hiding this comment.
Updated
| } | ||
|
|
||
| if model := resp.Model; model != nil { | ||
| state.Name = id.StartStopScheduleName |
There was a problem hiding this comment.
This isn't in the schema, so we don't need to set a value for this into state
| state.Name = id.StartStopScheduleName |
There was a problem hiding this comment.
Updated
| func expandScheduleItemModelArray(inputList []ScheduleItemModel) *[]schedule.ScheduleItem { | ||
| var outputList []schedule.ScheduleItem | ||
| for _, v := range inputList { | ||
| input := v | ||
| output := schedule.ScheduleItem{ | ||
| StartDay: input.StartDay, | ||
| StartTime: input.StartTime, | ||
| StopDay: input.StopDay, | ||
| StopTime: input.StopTime, | ||
| } | ||
|
|
||
| outputList = append(outputList, output) | ||
| } | ||
| return &outputList | ||
| } |
There was a problem hiding this comment.
The model doesn't have this defined as a pointer so I don't see a need to make this return a pointer
| func expandScheduleItemModelArray(inputList []ScheduleItemModel) *[]schedule.ScheduleItem { | |
| var outputList []schedule.ScheduleItem | |
| for _, v := range inputList { | |
| input := v | |
| output := schedule.ScheduleItem{ | |
| StartDay: input.StartDay, | |
| StartTime: input.StartTime, | |
| StopDay: input.StopDay, | |
| StopTime: input.StopTime, | |
| } | |
| outputList = append(outputList, output) | |
| } | |
| return &outputList | |
| } | |
| func expandScheduleItemModelArray(inputList []ScheduleItemModel) []schedule.ScheduleItem { | |
| var outputList []schedule.ScheduleItem | |
| for _, v := range inputList { | |
| input := v | |
| output := schedule.ScheduleItem{ | |
| StartDay: input.StartDay, | |
| StartTime: input.StartTime, | |
| StopDay: input.StopDay, | |
| StopTime: input.StopTime, | |
| } | |
| outputList = append(outputList, output) | |
| } | |
| return outputList | |
| } |
There was a problem hiding this comment.
Updated
| func flattenScheduleItemModelArray(inputList *[]schedule.ScheduleItem) []ScheduleItemModel { | ||
| var outputList []ScheduleItemModel | ||
| if inputList == nil { | ||
| return outputList | ||
| } | ||
| for _, input := range *inputList { | ||
| output := ScheduleItemModel{ | ||
| StartDay: input.StartDay, | ||
| StartTime: input.StartTime, | ||
| StopDay: input.StopDay, | ||
| StopTime: input.StopTime, | ||
| } | ||
|
|
||
| outputList = append(outputList, output) | ||
| } | ||
| return outputList | ||
| } |
There was a problem hiding this comment.
Same here
| func flattenScheduleItemModelArray(inputList *[]schedule.ScheduleItem) []ScheduleItemModel { | |
| var outputList []ScheduleItemModel | |
| if inputList == nil { | |
| return outputList | |
| } | |
| for _, input := range *inputList { | |
| output := ScheduleItemModel{ | |
| StartDay: input.StartDay, | |
| StartTime: input.StartTime, | |
| StopDay: input.StopDay, | |
| StopTime: input.StopTime, | |
| } | |
| outputList = append(outputList, output) | |
| } | |
| return outputList | |
| } | |
| func flattenScheduleItemModelArray(inputList []schedule.ScheduleItem) []ScheduleItemModel { | |
| var outputList []ScheduleItemModel | |
| if inputList == nil { | |
| return outputList | |
| } | |
| for _, input := range *inputList { | |
| output := ScheduleItemModel{ | |
| StartDay: input.StartDay, | |
| StartTime: input.StartTime, | |
| StopDay: input.StopDay, | |
| StopTime: input.StopTime, | |
| } | |
| outputList = append(outputList, output) | |
| } | |
| return outputList | |
| } |
There was a problem hiding this comment.
Updated
| if response.WasNotFound(resp.HttpResponse) { | ||
| return utils.Bool(false), nil | ||
| } | ||
| return nil, fmt.Errorf("retrieving %s: %+v", id, err) | ||
| } | ||
| return utils.Bool(resp.Model != nil), nil |
There was a problem hiding this comment.
If we get any error while running this function to check whether the resource exists we should return it
| if response.WasNotFound(resp.HttpResponse) { | |
| return utils.Bool(false), nil | |
| } | |
| return nil, fmt.Errorf("retrieving %s: %+v", id, err) | |
| } | |
| return utils.Bool(resp.Model != nil), nil | |
| return nil, fmt.Errorf("retrieving %s: %+v", id, err) | |
| } | |
| return pointer.To(resp.Model != nil), nil |
There was a problem hiding this comment.
Updated
| * `start_day` - (Required) Start day. | ||
|
|
||
| * `start_time` - (Required) Start time. | ||
|
|
||
| * `stop_day` - (Required) Stop day. | ||
|
|
||
| * `stop_time` - (Required) Stop time. |
There was a problem hiding this comment.
start_day and stop_day have validation for accepted values, can we extend the description here to include the possible values for these properties.
Also for start_time and stop_time, an example of the format we're expecting here would be helpful.
There was a problem hiding this comment.
Updated
Update mssql_managed_instance_sql_start_stop_schedule.html.markdown Update mssql_managed_instance_sql_start_stop_schedule.html.markdown
hqhqhqhqhqhqhqhqhqhqhq
force-pushed
the
feat/sql-StartStopManagedInstanceSchedules
branch
from
836c0fe to
72f2f41
Compare
There was a problem hiding this comment.
could you take a look at the test failure:
------- Stdout: -------
=== RUN TestAccMsSqlManagedInstanceStartStopSchedule_update
=== PAUSE TestAccMsSqlManagedInstanceStartStopSchedule_update
=== CONT TestAccMsSqlManagedInstanceStartStopSchedule_update
testcase.go:173: Step 4/6 error: Error running apply: exit status 1
Error: updating Managed Instance (Subscription: "*******"
Resource Group Name: "acctestRG1-sql-250110074135832701"
Managed Instance Name: "acctestsqlserver250110074135832701"): unexpected status 400 (400 Bad Request) with error: ManagementServiceStartStopManagedInstanceScheduleOverlap: One or multiple schedule items have a time span overlap. Please correct your schedule and submit again.
with azurerm_mssql_managed_instance_start_stop_schedule.test,
on terraform_plugin_test.tf line 304, in resource "azurerm_mssql_managed_instance_start_stop_schedule" "test":
304: resource "azurerm_mssql_managed_instance_start_stop_schedule" "test" {
updating Managed Instance (Subscription:
"*******"
Resource Group Name: "acctestRG1-sql-250110074135832701"
Managed Instance Name: "acctestsqlserver250110074135832701"): unexpected
status 400 (400 Bad Request) with error:
ManagementServiceStartStopManagedInstanceScheduleOverlap: One or multiple
schedule items have a time span overlap. Please correct your schedule and
submit again.
--- FAIL: TestAccMsSqlManagedInstanceStartStopSchedule_update (6545.41s)
FAIL
|
This PR is being labeled as "stale" because it has not been updated for 30 or more days. If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone. If you need some help completing this PR, please leave a comment letting us know. Thank you! |
Community Note
Description
Add new resource
azurerm_mssql_managed_instance_start_stop_schedule-> setting up start stop schedule formssql_managed_instanceresource.PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Done testing by building the repo and running terraform script with the setting. There needs some fix for
mssql_managed_instancetesting first as there are issues in related acc tests and this resource depends on themssql_managed_instance` resource.
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_mssql_managed_instance_start_stop_scheduleThis is a (please select all that apply):
Related Issue(s)
Note
If this PR changes meaningfully during the course of review please update the title and description as required.