resource_arm_sql_server: switch dependency from riviera to azure-sdk-for-go #189
Conversation
There was a problem hiding this comment.
Hey @sebastus
Thanks for this PR - I've reviewed and left some comments in-line - but this is off to a good start :)
Thanks!
azurerm/resource_arm_sql_server.go
Outdated
| return fmt.Errorf("Error creating SQL Server: %s", createResponse.Error) | ||
| if serverVersion == "" { | ||
| return fmt.Errorf("Invalid server version provided. It must be one of 12.0 or 2.0, passed as string: %s", version) | ||
| } |
There was a problem hiding this comment.
we can remove this in favour of adding validation here
i.e.
ValidateFunc: validation.StringInSlice([]string {
string(sql.OneTwoFullStopZero),
string(sql.TwoFullStopZero),
}, true)
azurerm/resource_arm_sql_server.go
Outdated
| serverVersion = sql.OneTwoFullStopZero | ||
| } | ||
| if version == string(sql.TwoFullStopZero) { | ||
| serverVersion = sql.TwoFullStopZero | ||
| } |
There was a problem hiding this comment.
we can replace all of this with:
version := sql.ServerVersion(version)
There was a problem hiding this comment.
(and we might as well move this cast into the properties assignment below tbh)
azurerm/resource_arm_sql_server.go
Outdated
| readResponse, err := readRequest.Execute() | ||
| //last parameter is set to empty to allow updates to records after creation | ||
| // (per SDK, set it to '*' to prevent updates, all other values are ignored) | ||
| result, err := sqlServersClient.CreateOrUpdate(resGroup, name, parameters) |
There was a problem hiding this comment.
can we remove these comments as they're not valid in this context? :)
azurerm/resource_arm_sql_server.go
Outdated
| d.Set("name", name) | ||
| d.Set("resource_group_name", resGroup) | ||
| d.Set("version", string(serverVersion)) | ||
| d.Set("location", *result.Location) |
There was a problem hiding this comment.
can we update location to use the azureRMNormalizeLocation(*resp.Location) method? this helps ensure it's stored consistently in the statefile
azurerm/resource_arm_sql_server.go
Outdated
|
|
||
| result, error := sqlServersClient.Delete(resGroup, name) | ||
| if result.Response.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("Error deleting SQL Server %s: %s", name, error) |
There was a problem hiding this comment.
can we update the second argument to be %+v?
| if err != nil { | ||
| return fmt.Errorf("Bad: GetServer: %s", err) | ||
| return nil |
There was a problem hiding this comment.
shouldn't we be returning this error here, and checking to see if it's a 404 above?
There was a problem hiding this comment.
we should take this up with the team. this pattern is throughout AzureRM
There was a problem hiding this comment.
I might be mis-reading your point - but I believe returning an error from the SDK this is the correct use-case, given it's an unsuccessful (i.e. non-200) state?
| return err | ||
| } | ||
| } else { | ||
| return fmt.Errorf("Bad type assertion for error returned from sqlServersClient.") | ||
| } |
There was a problem hiding this comment.
As far as I know there's no guarantees given on Error types returned by the SDK - so I think this might be safer + simpler to instead read the status code from the Response object? Here's an example of a recently refactored test which shows what I mean - what do you think?
(an aside: I think this would read cleaner as an if statement anyway, given we're only concerned with a 404 here?)
|
@tombuildsstuff Made the changes you requested. |
| metadata := expandTags(tags) | ||
|
|
||
| parameters := sql.Server{ | ||
| Location: &location, |
There was a problem hiding this comment.
@sebastus I was doing some edge-case testing on this - we can't submit the FQDN since it's only returned as a Computed property (as such I've removed this)
| // if the name is in-use, Azure returns a 409 "Unknown Service Error" which is a bad UX | ||
| if responseWasConflict(response.Response) { | ||
| return fmt.Errorf("SQL Server names need to be globally unique and '%s' is already in use.", name) | ||
| } |
There was a problem hiding this comment.
@sebastus heads up that doing some edge-case testing I realised Azure returns a "409 Unknown Service Error" which isn't such a great UX, so I've added this to be more descriptive (appears the name needs to be globally unique as it's used in the FQDN)
There was a problem hiding this comment.
Hey @sebastus
Thanks for pushing those updates - doing some testing on this I'd noticed some minor things/edge-cases and have pushed a couple of commits to address those.
The tests pass and this LGTM:
$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMSqlServer
=== RUN TestAccAzureRMSqlServer_importBasic
--- PASS: TestAccAzureRMSqlServer_importBasic (126.10s)
=== RUN TestAccAzureRMSqlServer_basic
--- PASS: TestAccAzureRMSqlServer_basic (104.16s)
=== RUN TestAccAzureRMSqlServer_withTags
--- PASS: TestAccAzureRMSqlServer_withTags (130.70s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 360.980s
Thanks for this!
|
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.