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

Add charset and collation to google_sql_database. #183

Merged
merged 8 commits into from
Jul 17, 2017
Merged

Add charset and collation to google_sql_database. #183

merged 8 commits into from
Jul 17, 2017

Conversation

ubschmidt2
Copy link
Contributor

@ubschmidt2 ubschmidt2 commented Jul 10, 2017 •
edited
Loading

Fixes #179.

@ubschmidt2 ubschmidt2 changed the title WIP Add charset and collation to google_sql_database. WIP: Add charset and collation to google_sql_database. Jul 10, 2017
@ubschmidt2
Copy link
Contributor Author

Added a trivial acceptance test. Happy to improve, please advise.

$ gmake testacc GOOGLE_USE_DEFAULT_CREDENTIALS=1 GOOGLE_PROJECT=ubschmidt GOOGLE_REGION=us-central1 GOOGLE_XPN_HOST_PROJECT=a TEST=./google TESTARGS='-run=TestAccGoogleSqlDatabase_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleSqlDatabase_basic -timeout 120m
=== RUN TestAccGoogleSqlDatabase_basic
--- PASS: TestAccGoogleSqlDatabase_basic (85.58s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 85.596s

@ubschmidt2 ubschmidt2 changed the title WIP: Add charset and collation to google_sql_database. Add charset and collation to google_sql_database. Jul 12, 2017
rileykarson
rileykarson requested changes Jul 14, 2017
View reviewed changes
Copy link
Collaborator

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

I left a few comments - they're all pretty minor, just moving a few things around and fleshing out the example in the docs with your new fields.

google/resource_sql_database.go Outdated
@@ -62,6 +75,14 @@ func resourceSqlDatabaseCreate(d *schema.ResourceData, meta interface{}) error {
Instance: instance_name,
}

if v, ok := d.GetOk("charset"); ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we have a default set, we should always get a value from this; we can change both values from d.GetOk to d.Get. We can also inline these values into the struct initializer of db.

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.

google/resource_sql_database.go Outdated
Instance: instance_name,
}

if v, ok := d.GetOk("charset"); ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above - we will always have a value present, so we can use d.Get instead of d.GetOk, and inline these into db

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.

google/resource_sql_database_test.go
"google_sql_database.database", &database),
),
},
resource.TestStep{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's split the charset and collation parts into their own test and leave _basic as a single-step test; that way, we can keep the testing for Update out of _basic.

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.

website/docs/r/sql_database.html.markdown Outdated
@@ -37,6 +37,10 @@ The following arguments are supported:

* `instance` - (Required) The name of containing instance.

* `charset` - (Optional) The MySQL charset value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: These entries should live under the horizontal divider with other optional fields.

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.

website/docs/r/sql_database.html.markdown Outdated
@@ -37,6 +37,10 @@ The following arguments are supported:

* `instance` - (Required) The name of containing instance.

* `charset` - (Optional) The MySQL charset value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind adding the charset and collation fields to the example at the top of the page? (Apologies for commenting here - Github doesn't like comments on unchanged lines)

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. Also documented defaults in argument reference.

@ubschmidt2
Copy link
Contributor Author

Thanks for the review!

@rileykarson
Copy link
Collaborator

LGTM, thanks!

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleSqlDatabase_ -timeout 120m
=== RUN   TestAccGoogleSqlDatabase_importBasic
--- PASS: TestAccGoogleSqlDatabase_importBasic (61.05s)
=== RUN   TestAccGoogleSqlDatabase_basic
--- PASS: TestAccGoogleSqlDatabase_basic (60.69s)
=== RUN   TestAccGoogleSqlDatabase_update
--- PASS: TestAccGoogleSqlDatabase_update (67.55s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	189.436s

@rileykarson rileykarson merged commit 9e5aa27 into hashicorp:master Jul 17, 2017
@ubschmidt2 ubschmidt2 mentioned this pull request Jul 20, 2017
Merged
negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
@ubschmidt2 @rileykarson
Add charset and collation to google_sql_database. (hashicorp#183)
3835abf 
* Add charset and collation to google_sql_database.

* Add documentation for charset, collation attributes.

* Extend the existing acceptance test to also cover charset and collation.

* Charset and collation always have a value present. Also inline.

* Move charset and collation to optional arguments.

* Add charset and collection to the example.

* Document charset and collation defaults.

* Keep TestAccGoogleSqlDatabase_basic as is, add TestAccGoogleSqlDatabase_update.
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
@ubschmidt2 @rileykarson
Add charset and collation to google_sql_database. (hashicorp#183)
3fb0b55 
* Add charset and collation to google_sql_database.

* Add documentation for charset, collation attributes.

* Extend the existing acceptance test to also cover charset and collation.

* Charset and collation always have a value present. Also inline.

* Move charset and collation to optional arguments.

* Add charset and collection to the example.

* Document charset and collation defaults.

* Keep TestAccGoogleSqlDatabase_basic as is, add TestAccGoogleSqlDatabase_update.
@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

@rileykarson rileykarson rileykarson requested changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Support setting charset and collation for google_sql_database resource
2 participants
@ubschmidt2 @rileykarson