resource_arm_sql_database: switch dependency from riviera to azure-sdk-for-go #191
Conversation
There was a problem hiding this comment.
Hey @sebastus
Thanks for this PR - apologies for the delayed review on this, I'd started reviewed it but not hit submit :(
I've taken a look through and left some comments but this is off to a good start :)
Thanks!
| Default: "Default", | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Default: "Default", |
There was a problem hiding this comment.
can we use the enum in the SDK for this? i.e. string(sql.Default)
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Default: "Default", | ||
| ValidateFunc: validateArmSqlDatabaseCreateMode, |
There was a problem hiding this comment.
we should be able to remove this in favour of an in-line validation function:
ValidateFunc: validation.StringInSlice([]string{
string(sql.Copy),
string(sql.RestoreLongTermRetentionBackup),
string(sql.Default),
string(sql.NonReadableSecondary),
string(sql.OnlineSecondary),
string(sql.PointInTimeRestore),
string(sql.Recovery),
string(sql.Restore),
string(sql.RestoreLongTermRetentionBackup),
}, true)
There was a problem hiding this comment.
can we add a reference to the DiffSuppressFunc which will ignore differences in case here:
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
| if v, ok := d.GetOk("collation"); ok { | ||
| command.Collation = azure.String(v.(string)) | ||
| } | ||
| edition := sql.DatabaseEdition(d.Get("edition").(string)) |
There was a problem hiding this comment.
given this field is optional, I think we want to optionally assign it to props below? i.e.
if v, ok := d.GetOk("edition"); ok {
props.Edition = sql.DatabaseEdition(v.(string))
}
| requestedServiceObjectiveNameString := d.Get("requested_service_objective_name").(string) | ||
| var requestedServiceObjectiveName sql.ServiceObjectiveName | ||
|
|
||
| if requestedServiceObjectiveIDString != "" && requestedServiceObjectiveNameString == "" { |
There was a problem hiding this comment.
we can use the conflicts_with option in the Schema to handle this for us automatically, i.e.:
"requested_service_objective_id": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ConflictsWith: []string{ "requested_service_objective_name" },
},
"requested_service_objective_name": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ConflictsWith: []string{ "requested_service_objective_id" },
},
and then just have if statements for each field, and assign the value directly to props as needed, rather than checking the state of both:
if v, ok := d.Get("requested_service_objective_id").(string); ok {
rsoid, err := uuid.FromString(v.(string))
if err != nil {
return err
}
props.RequestedServiceObjectiveID = &rsoid
}
if v, ok := d.Get("requested_service_objective_name").(string); ok {
props.RequestedServiceObjectiveID = sql.ServiceObjectiveName(v.(string))
}
| createResponse, err := createRequest.Execute() | ||
| if err != nil { | ||
| return fmt.Errorf("Error creating SQL Database: %s", err) | ||
| if requestedServiceObjectiveNameString == "ElasticPool" { |
There was a problem hiding this comment.
is there an enum/constant we can use for this?
| databaseName := id.Path["databases"] | ||
|
|
||
| result, error := sqlDatabasesClient.Delete(resGroup, serverName, databaseName) | ||
| if result.Response.StatusCode != http.StatusOK { |
There was a problem hiding this comment.
| @@ -257,3 +287,20 @@ func validateArmSqlDatabaseEdition(v interface{}, k string) (ws []string, errors | |||
| } | |||
| return | |||
| } | |||
|
|
|||
| func validateArmSqlDatabaseCreateMode(v interface{}, k string) (ws []string, errors []error) { | |||
There was a problem hiding this comment.
(if we switch to using the validation.StringInStrings and the enum's/constants above - I think we can remove this?)
| if !readResponse.IsSuccessful() { | ||
| return fmt.Errorf("Bad: GetDatabase: %s", readResponse.Error) | ||
|
|
||
| if resp.StatusCode == http.StatusNotFound { |
There was a problem hiding this comment.
can we update this to be within the err statement above, as 404 should be being returned as an error
| if err != nil { | ||
| return fmt.Errorf("Bad: GetDatabase: %s", err) | ||
| return nil |
There was a problem hiding this comment.
can we update this to return the error, and add a check for if 404 { return nil } - given a 404 is an error type?
|
|
||
| for _, rs := range s.RootModule().Resources { | ||
| if rs.Type != "azurerm_sql_database" { | ||
| if rs.Type != "azurerm_sql_database_record" { |
There was a problem hiding this comment.
can we change this back to azurerm_sql_database, given that's the resource name?
|
Hey @sebastus I've been working on a similar task which requires the Riviera SDK to be removed - as such I've rolled your PR's (#191 #190 and #178) into a new PR - which also removes the bulk of the Riviera SDK; I hope you don't mind (and we really appreciate your efforts here) - and this is visible in #289. As such I'm going to close this PR for the moment in favour of #289. Thanks! |
|
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
locked and limited conversation to collaborators
No description provided.