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

resource_arm_sql_server: switch dependency from riviera to azure-sdk-for-go #189

Merged
merged 8 commits into from
Aug 18, 2017
Merged

resource_arm_sql_server: switch dependency from riviera to azure-sdk-for-go #189

merged 8 commits into from
Aug 18, 2017

Conversation

sebastus
Copy link
Contributor

No description provided.

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.

Hey @sebastus

Thanks for this PR - I've reviewed and left some comments in-line - but this is off to a good start :)

Thanks!

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)
}
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 remove this in favour of adding validation here

i.e.

ValidateFunc: validation.StringInSlice([]string {
  string(sql.OneTwoFullStopZero),
  string(sql.TwoFullStopZero),
}, true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

serverVersion = sql.OneTwoFullStopZero
}
if version == string(sql.TwoFullStopZero) {
serverVersion = sql.TwoFullStopZero
}
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 replace all of this with:

version := sql.ServerVersion(version)

Copy link
Contributor

Choose a reason for hiding this comment

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

(and we might as well move this cast into the properties assignment below tbh)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

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)
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 remove these comments as they're not valid in this context? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

d.Set("name", name)
d.Set("resource_group_name", resGroup)
d.Set("version", string(serverVersion))
d.Set("location", *result.Location)
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 update location to use the azureRMNormalizeLocation(*resp.Location) method? this helps ensure it's stored consistently in the statefile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


result, error := sqlServersClient.Delete(resGroup, name)
if result.Response.StatusCode != http.StatusOK {
return fmt.Errorf("Error deleting SQL Server %s: %s", name, error)
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 update the second argument to be %+v?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

if err != nil {
return fmt.Errorf("Bad: GetServer: %s", err)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we be returning this error here, and checking to see if it's a 404 above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should take this up with the team. this pattern is throughout AzureRM

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?)

@sebastus
Copy link
Contributor Author

@tombuildsstuff Made the changes you requested.

metadata := expandTags(tags)

parameters := sql.Server{
Location: &location,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@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)

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.

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!

@tombuildsstuff tombuildsstuff merged commit e4760be into hashicorp:master Aug 18, 2017
tombuildsstuff added a commit that referenced this pull request Aug 18, 2017
tombuildsstuff added a commit that referenced this pull request Aug 21, 2017
@sebastus sebastus deleted the resource_arm_sql_server branch September 6, 2017 21:36
@ghost
Copy link

ghost commented Apr 1, 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 Apr 1, 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