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

Add time partitioning field to google_bigquery_table resource #1240

Conversation

amatellanes
Copy link
Contributor

Add time partitioning field to google_bigquery_table resource. This field is available on type TimePartitioning from package bigquery:

type TimePartitioning struct {
    ...

    // If empty, the table is partitioned by pseudo column '_PARTITIONTIME'; if set, the
    // table is partitioned by this field. The field must be a top-level TIMESTAMP or
    // DATE field. Its mode must be NULLABLE or REQUIRED.
    Field string
}

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Mind adding a test (or updating an existing one) to use the new field?

@@ -428,6 +440,11 @@ func expandTimePartitioning(configured interface{}) *bigquery.TimePartitioning {

func flattenTimePartitioning(tp *bigquery.TimePartitioning) []map[string]interface{} {
result := map[string]interface{}{"type": tp.Type}
result["field"] = tp.Type
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 this be tp.Field?

@amatellanes
Copy link
Contributor Author

@danawillow I have fix flattenTimePartitioning function to retrieve field. Also I have created TestAccBigQueryTable_TimePartitioningField test to check time partitioning field but I'm not really sure if I should have added this test to an existing one... perhaps TestAccBigQueryTable_Basic...

@danawillow
Copy link
Contributor

Thanks! Yeah I think having it in basic would work fine. Do you mind also adding an import step to the test? We're trying to standardize on as many tests as possible having that step since it lets us check that all the fields match. It would look like this:

			{
				ResourceName:        "google_bigquery_table.test",
				ImportState:         true,
				ImportStateVerify:   true,
			},

@amatellanes
Copy link
Contributor Author

Hi @danawillow

Changes done.

@danawillow
Copy link
Contributor

Hey @amatellanes, I'm not sure if you were intending to have the test update the existing table, but by having the two steps in the same test that's what it tried to do. It sounds like from this error message that you can't update that field. If that's true, can you make it ForceNew?

$ make testacc TEST=./google TESTARGS='-run=TestAccBigQueryTable_Basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccBigQueryTable_Basic -timeout 120m
=== RUN   TestAccBigQueryTable_Basic
=== PAUSE TestAccBigQueryTable_Basic
=== CONT  TestAccBigQueryTable_Basic
--- FAIL: TestAccBigQueryTable_Basic (6.64s)
	testing.go:513: Step 2 error: Error applying: 1 error(s) occurred:

		* google_bigquery_table.test: 1 error(s) occurred:

		* google_bigquery_table.test: googleapi: Error 400: Cannot change partitioning spec for a partitioned table., invalid
FAIL
FAIL	github.com/terraform-providers/terraform-provider-google/google	6.847s
make: *** [testacc] Error 1

@amatellanes
Copy link
Contributor Author

I've just marked the field as ForceNew. Since we cannot update partitioning field for a partitioned table, should I create a new test or modify testAccBigQueryTable to add the field Field to time_partitioning block?

@amatellanes amatellanes force-pushed the add-time-partitioning-field-to-google-bigquery-table-resource branch from 028f719 to 192bf31 Compare March 30, 2018 00:22
@amatellanes amatellanes force-pushed the add-time-partitioning-field-to-google-bigquery-table-resource branch from c840110 to 15c6b51 Compare March 30, 2018 00:24
@danawillow
Copy link
Contributor

I would just modify testAccBigQueryTable but I don't care particularly strongly

@amatellanes
Copy link
Contributor Author

Finally I've chosen to modify testAccBigQueryTable.

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

$ make testacc TEST=./google TESTARGS='-run=TestAccBigQueryTable_Basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccBigQueryTable_Basic -timeout 120m
=== RUN   TestAccBigQueryTable_Basic
=== PAUSE TestAccBigQueryTable_Basic
=== CONT  TestAccBigQueryTable_Basic
--- PASS: TestAccBigQueryTable_Basic (5.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	5.753s

Thanks @amatellanes! Looks good!

@danawillow danawillow merged commit 1f6ffa0 into hashicorp:master Mar 30, 2018
@amatellanes amatellanes deleted the add-time-partitioning-field-to-google-bigquery-table-resource branch March 30, 2018 21:59
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
…orp#1240)

* Add time partitioning field to google_bigquery_table resource

* Fix flatten time partitioning field to google_bigquery_table resource

* Add resource bigquery table time partitioning field test

* Move resource bigquery table time partitioning field test to basic

* Add step to check that all the fields match

* Mark resource bigquery table time partitioning field as ForceNew

* Add time partitioning field test to testAccBigQueryTable config
@ghost
Copy link

ghost commented Nov 19, 2018

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Nov 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants