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

Added new resource azurerm_scheduler_job_collection #963

Merged
merged 4 commits into from
Mar 13, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Mar 11, 2018

No description provided.

@katbyte katbyte added this to the 1.3.0 milestone Mar 11, 2018
@katbyte katbyte requested a review from tombuildsstuff March 11, 2018 21:47
@katbyte katbyte force-pushed the f-scheduler_job_collection branch 2 times, most recently from 189fac6 to 134c351 Compare March 11, 2018 21:50
@katbyte katbyte force-pushed the f-scheduler_job_collection branch from 134c351 to 76c27ee Compare March 11, 2018 21:50
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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",
Copy link
Contributor

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
Copy link
Contributor

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": {
Copy link
Contributor

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),
Copy link
Contributor

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?

Copy link
Collaborator Author

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 = &quota
Copy link
Contributor

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})
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Collaborator Author

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
Copy link
Contributor

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"
Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Scheduler Job collections*

@katbyte
Copy link
Collaborator Author

katbyte commented Mar 13, 2018

Thanks for the review! Pushed requested changes.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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)
Copy link
Contributor

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
}
Copy link
Contributor

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?

@katbyte katbyte force-pushed the f-scheduler_job_collection branch 4 times, most recently from 12cb989 to 790b30c Compare March 13, 2018 21:11
@katbyte katbyte force-pushed the f-scheduler_job_collection branch from 790b30c to c4f7781 Compare March 13, 2018 21:13
@katbyte
Copy link
Collaborator Author

katbyte commented Mar 13, 2018

Tests pass:
screen shot 2018-03-13 at 15 34 43

@katbyte katbyte merged commit 21929db into master Mar 13, 2018
@katbyte katbyte deleted the f-scheduler_job_collection branch March 13, 2018 22:35
katbyte added a commit that referenced this pull request Mar 13, 2018
@ghost
Copy link

ghost commented Mar 31, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants