-
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: azurerm_dynatrace_monitor
#27432
Conversation
country := v["country"].(string) | ||
emailAddress := v["email"].(string) | ||
firstName := v["first_name"].(string) | ||
lastName := v["last_name"].(string) | ||
phoneNumber := v["phone_number"].(string) | ||
|
||
return []UserInfo{ | ||
{ | ||
Country: country, | ||
EmailAddress: emailAddress, | ||
FirstName: firstName, | ||
LastName: lastName, | ||
PhoneNumber: phoneNumber, | ||
}, | ||
} |
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 we please just inline these?
country := v["country"].(string) | |
emailAddress := v["email"].(string) | |
firstName := v["first_name"].(string) | |
lastName := v["last_name"].(string) | |
phoneNumber := v["phone_number"].(string) | |
return []UserInfo{ | |
{ | |
Country: country, | |
EmailAddress: emailAddress, | |
FirstName: firstName, | |
LastName: lastName, | |
PhoneNumber: phoneNumber, | |
}, | |
} | |
return []UserInfo{ | |
{ | |
Country: v["country"].(string), | |
EmailAddress: v["email"].(string), | |
FirstName: v["first_name"].(string), | |
LastName: lastName, | |
PhoneNumber: phoneNumber, | |
}, | |
} |
etc
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.
Will do.
"MONTHLY", | ||
"WEEKLY", |
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 these be an enum?
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.
They are not enums.
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.
Other APIs correctly make these an enum (ie sql_license_type) so i presume this is a swagger oversite, could we create a quick rest api specs issue and the link that inline?
"PAYG", | ||
"COMMITTED", |
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 this
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, these are not enums.
"email": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ValidateFunc: validation.StringIsNotEmpty, |
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 can propertly validate this is an email?
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.
Will do.
"plan": SchemaPlanData(), | ||
|
||
"user": SchemaUserInfo(), |
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 there a reason we are pulling these out into a function? they are only used here so could we please inline them to be consistant with the rest of the provider?
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.
Will inline them.
"Active", | ||
"Suspended", |
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 an enum?
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.
Will change.
|
||
A `plan` block supports the following: | ||
|
||
* `billing_cycle` - (Optional) Different billing cycles. Possible values are `MONTHLY`, `WEEKLY`. |
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.
comma's are not used for two item list
* `billing_cycle` - (Optional) Different billing cycles. Possible values are `MONTHLY`, `WEEKLY`. | |
* `billing_cycle` - (Optional) Different billing cycles. Possible values are `MONTHLY` and `WEEKLY`. |
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.
Will change.
|
||
* `identity` - (Required) The kind of managed identity assigned to this resource. A `identity` block as defined below. | ||
|
||
* `marketplace_subscription` - (Required) Flag specifying the Marketplace Subscription Status of the resource. If payment is not made in time, the resource will go in Suspended state. Possible values are `Active`, `Suspended`. |
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
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.
Will change.
|
||
* `plan` - (Required) Plan id as published by Dynatrace. | ||
|
||
* `usage_type` - (Optional) Different usage type. Possible values are `PAYG`, `COMMITTED`. |
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
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.
Will change.
4c2f4c1
to
c31798c
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.
2 minor comments + the rest API specs for the missing enum but otherwise LGTM once they are resolved 🏷️
monitoringStatus = monitors.MonitoringStatusDisabled | ||
} | ||
monitorsProps := monitors.MonitorProperties{ | ||
MarketplaceSubscriptionStatus: pointer.To(marketplaceSubscriptionServiceStatus), |
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 can be
MarketplaceSubscriptionStatus: pointer.To(marketplaceSubscriptionServiceStatus), | |
MarketplaceSubscriptionStatus: pointer.To(monitors.MarketplaceSubscriptionStatus(model.MarketplaceSubscriptionStatus)), |
?
} | ||
if model := resp.Model; model != nil { |
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.
} | |
if model := resp.Model; model != nil { | |
} | |
if model := resp.Model; model != nil { |
MarketplaceSubscriptionStatus: string(*props.MarketplaceSubscriptionStatus), | ||
Identity: identityProps, | ||
PlanData: FlattenDynatracePlanData(props.PlanData), | ||
UserInfo: FlattenDynatraceUserInfo(userInfo), |
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 should be
UserInfo: FlattenDynatraceUserInfo(userInfo), | |
UserInfo: FlattenDynatraceUserInfo(metadata.ResourceData.Get("user").([]interface{})), |
?
@jiaweitao001 also don't forget to post test evidence |
Hi @katbyte , the test results are listed in the description note at the top. |
@jiaweitao001 - you need to rerun the tests with all the changes/commits made to ensure it still works, and tests should be a comment after the changes have been made. Please re-run the tests and add as a comment. |
Latest test results:
|
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 LGTM ☕
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_dynatrace_monitor
This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.
As discussed several months ago, since the tests of this resource cost too much, I'll perform tests on the subs provided by the service team. Here's the test results.