-
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
Added new resource azurerm_scheduler_job_collection #963
Conversation
189fac6
to
134c351
Compare
134c351
to
76c27ee
Compare
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.
left some comments but this is mostly looking good 👍
"state": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Default: "enabled", |
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 make this ‘string(scheduler.Enabled)’
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
|
||
//max_job_occurrence doesn't seem to do anything and always remains empty |
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.
Given that I guess this field is deprecated (or the API’s broken) - so we can ignore it for now 👍
}, | ||
|
||
//should this be max_retry_interval ? given that is what the documentation implies | ||
"max_recurrence_interval": { |
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.
Yep, that makes sense to rename this to be clearer
string(scheduler.Enabled), | ||
string(scheduler.Suspended), | ||
string(scheduler.Disabled), | ||
string(scheduler.Deleted), |
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 possible to set Deleted?
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.
Surprisingly, yes! But it probably does make sense to prevent it.
recurrence.Interval = utils.Int32(int32(v)) | ||
} | ||
|
||
properties.Quota = "a |
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 can we pull this out into an “expandSchedulerJobCollectionQuota” function
quotaBlock["max_recurrence_frequency"] = string(recurrence.Frequency) | ||
} | ||
|
||
d.Set("quota", []interface{}{quotaBlock}) |
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 pull this out into a “flattenSchedulerJobCollectionQuota” function and check for errors when assigning it?
return resourceArmSchedulerJobCollectionPopulate(d, resourceGroup, &collection) | ||
} | ||
|
||
func resourceArmSchedulerJobCollectionPopulate(d *schema.ResourceData, resourceGroup string, collection *scheduler.JobCollectionDefinition) error { |
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.
We should be able to move this into the Read method?
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 separated it out like this so there wouldn't be two read requests after a create/update.
name := rs.Primary.Attributes["name"] | ||
resourceGroup := rs.Primary.Attributes["resource_group_name"] | ||
|
||
client := testAccProvider.Meta().(*ArmClient).applicationSecurityGroupsClient |
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 use the right client here? :)
|
||
func testAccAzureRMSchedulerJobCollection_complete(rInt int, location string) string { | ||
return testAccAzureRMSchedulerJobCollection_basic(rInt, location, ` | ||
state = "disabled" |
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 combine this into one function?
|
||
## Import | ||
|
||
Application Security Groups can be imported using the `resource id`, e.g. |
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.
Scheduler Job collections*
Thanks for the review! Pushed requested changes. |
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.
a couple of minor comments for consistency purposes, but this otherwise LGTM 👍
d.Set("state", string(properties.State)) | ||
|
||
if err := d.Set("quota", flattenAzureArmSchedulerJobCollectionQuota(properties.Quota)); err != nil { | ||
return fmt.Errorf("Error flattening quoto for Job Collection %q (Resource Group %q): %+v", collection.Name, resourceGroup, 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 quoto
-> quota
max_recurrence_frequency = "hour" | ||
max_retry_interval = 10 | ||
max_job_count = 10 | ||
} |
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 separate this out into a separate configuration called testAccAzureRMSchedulerJobCollection_complete
to match the other tests?
12cb989
to
790b30c
Compare
790b30c
to
c4f7781
Compare
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 👉 [email protected]. Thanks! |
No description provided.