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.

Sign up for GitHub

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

Jump to bottom

scheduler_job_collection: Renamed max_retry_interval to max_recurrence_interval to better match azure #1218

Merged
merged 3 commits into from
May 14, 2018
Merged

scheduler_job_collection: Renamed max_retry_interval to max_recurrence_interval to better match azure #1218

merged 3 commits into from
May 14, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented May 8, 2018

No description provided.

@katbyte katbyte added this to the 1.5.0 milestone May 8, 2018
@katbyte katbyte requested a review from a team May 8, 2018 21:29
@tombuildsstuff tombuildsstuff modified the milestones: 1.5.0, 1.6.0 May 8, 2018
@katbyte katbyte force-pushed the b-r-scheduler_job_collection branch from 9dfd377 to cc2142e Compare May 9, 2018 05:35
@katbyte katbyte force-pushed the b-r-scheduler_job_collection branch from cc2142e to b243043 Compare May 9, 2018 05:35
@katbyte
Copy link
Collaborator Author

katbyte commented May 9, 2018

Tests pass:

TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMSchedulerJobCollection -timeout 180m
=== RUN   TestAccAzureRMSchedulerJobCollection_basic
--- PASS: TestAccAzureRMSchedulerJobCollection_basic (67.56s)
=== RUN   TestAccAzureRMSchedulerJobCollection_complete
--- PASS: TestAccAzureRMSchedulerJobCollection_complete (67.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	134.789s

tombuildsstuff
tombuildsstuff suggested changes May 14, 2018
View reviewed 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 points - but if we can clean up the comments this otherwise LGTM 👍

azurerm/resource_arm_scheduler_job_collection.go Outdated
@@ -69,6 +69,7 @@ func resourceArmSchedulerJobCollection() *schema.Resource {
"quota": {
Type: schema.TypeList,
Optional: true,
MinItems: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

if this isn't required then this can be removed

azurerm/data_source_scheduler_job_collection.go Outdated
@@ -102,7 +102,7 @@ func dataSourceArmSchedulerJobCollectionRead(d *schema.ResourceData, meta interf
}
d.Set("state", string(properties.State))

if err := d.Set("quota", flattenAzureArmSchedulerJobCollectionQuota(properties.Quota)); err != nil {
if err := d.Set("quota", flattenAzureArmSchedulerJobCollectionQuota(properties.Quota, d.Get("quota").([]interface{}))); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

in general we shouldn't access properties via d.Get in the Read method - since they can be absent when importing

azurerm/resource_arm_scheduler_job_collection.go Outdated
if v, ok := b["max_retry_interval"].(int); ok && v > 0 {
maxRecurrenceField = "max_retry_interval"
} //else max_recurrence_interval is in use, or we default to 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.

rather than doing this, we can instead just assign both of these values, since one's deprecated

tombuildsstuff
tombuildsstuff reviewed May 14, 2018
View reviewed 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.

as discussed IRL

azurerm/resource_arm_scheduler_job_collection.go Outdated
ValidateFunc: validation.IntAtLeast(1), //changes depending on the frequency, unknown maximums
Type: schema.TypeInt,
Optional: true,
Deprecated: "Renamed to match azure",
Copy link
Contributor

Choose a reason for hiding this comment

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

we can make this Computed and remove the ConflictsWith

azurerm/resource_arm_scheduler_job_collection.go Outdated
if v, ok := quotaBlock["max_retry_interval"].(int); ok && v > 0 {
quota.MaxRecurrence.Interval = utils.Int32(int32(v))
}
if v, ok := quotaBlock["max_recurrence_interval"].(int); ok && v > 0 {
quota.MaxRecurrence.Interval = utils.Int32(int32(v))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if v, ok := quotaBlock["max_recurrence_interval"]; ok {
  // use the new value
  quota.MaxRecurrence.Interval = utils.Int32(int32(v))
} elseif v, ok := quotaBlock["max_retry_interval"]; ok {
  // use the old value
  quota.MaxRecurrence.Interval = utils.Int32(int32(v))
}

tombuildsstuff
tombuildsstuff approved these changes May 14, 2018
View reviewed 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.

now LGTM 👍

@katbyte
Copy link
Collaborator Author

katbyte commented May 14, 2018

tests pass:

[15:54:05] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMSchedulerJobCollection
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMSchedulerJobCollection -timeout 180m
=== RUN   TestAccAzureRMSchedulerJobCollection_basic
--- PASS: TestAccAzureRMSchedulerJobCollection_basic (65.82s)
=== RUN   TestAccAzureRMSchedulerJobCollection_complete
--- PASS: TestAccAzureRMSchedulerJobCollection_complete (72.85s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	138.697s

@katbyte katbyte merged commit 2f0129d into master May 14, 2018
@katbyte katbyte deleted the b-r-scheduler_job_collection branch May 14, 2018 23:04
katbyte added a commit that referenced this pull request May 14, 2018
@katbyte
Update CHANGELOG.md to include #1218
db5b8d8
@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.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

Assignees
No one assigned
Labels
bug service/scheduler
Projects
None yet
Milestone
v1.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@katbyte @tombuildsstuff