New Resource: aws_athena_database #1922

atsushi-ishibashi · 2017-10-17T04:40:05Z

…form-provider-aws into resouce_aws_athena_database

atsushi-ishibashi · 2017-10-23T06:55:36Z

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAthenaDatabase'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAthenaDatabase -timeout 120m
=== RUN   TestAccAWSAthenaDatabase
--- PASS: TestAccAWSAthenaDatabase (73.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	73.535s

In testAccCheckAWSAthenaDatabaseDestroy, to check whether the database was deleted from sql result drop database <database-name>;, temporary s3 bucket was created and destroied.

atsushi-ishibashi · 2017-10-23T07:01:09Z

@radeksimko So far I couldn't developed resourceAwsAthenaDatabaseUpdate because I couldn't defined the best update flow.
When there is no data, it maybe ok that drop database, (only when bucket changed delete s3-bucket, create s3-bucket), create database. But when there are already data in the bucket, what is expected for UPDATE?
It'd be great if you could give me any ideas:bow:

atsushi-ishibashi · 2017-10-27T02:10:49Z

@radeksimko Could you give me feedback? I'd like to work #1893 🙇

radeksimko

Hey @atsushi-ishibashi
thanks for this PR. I left you some comments there, mostly related to code readability & reusability.

Let me know if anything's unclear.

radeksimko · 2017-10-27T10:41:51Z

aws/resource_aws_athena_database.go

+
+func resourceAwsAthenaDatabaseUpdate(d *schema.ResourceData, meta interface{}) error {
+	return nil
+}


We don't need to define the update function at all if there's nothing to update.

radeksimko · 2017-10-27T10:45:50Z

aws/resource_aws_athena_database.go

+	input := &athena.GetQueryExecutionInput{
+		QueryExecutionId: aws.String(qeid),
+	}
+	err := resource.Retry(5*time.Minute, func() *resource.RetryError {


We have a bit more suitable retry helper for this context here as the Athena query is stateful, see resource.StateChangeConf:

https://github.com/terraform-providers/terraform-provider-aws/blob/fb52fc7866f4107d1e0a341936d3b9531ff71a2e/aws/resource_aws_batch_job_queue.go#L64-L77

Thank you! I'll refer it:bow:

radeksimko · 2017-10-27T10:47:52Z

website/docs/r/athena_database.html.markdown

+  name = "database_name"
+  bucket = "${aws_s3_bucket.hoge.bucket}"
+
+  depends_on = ["aws_s3_bucket.hoge"]


This is unnecessary, the relationship is already defined by the reference 2 lines above.

radeksimko · 2017-10-27T10:48:35Z

aws/resource_aws_athena_database_test.go

+      name = "%s"
+      bucket = "${aws_s3_bucket.hoge.bucket}"
+
+      depends_on = ["aws_s3_bucket.hoge"]


This is unnecessary, the relationship is already defined by the reference 2 lines above.

radeksimko · 2017-10-27T10:52:29Z

aws/resource_aws_athena_database_test.go

+	})
+}
+
+func testAccCheckAWSAthenaDatabaseDestroy(s *terraform.State) error {


I think there's a bit of misunderstanding of the purpose of the CheckDestroy function. It's not supposed to create or destroy resources, it's merely supposed to check if they're destroyed and we can even scope it down just to the single resource being tested - i.e. Athena DB in this case. It's the testing framework's responsibility to do the cleanup by effectively calling terraform destroy after running the test.

After terraform destroy, query output destination s3 bucket is also deleted so I couldn't call StartQueryExecution to confirm whether the database is acctually deleted or not.
So in this case I create temporary s3 bucket as result destination and run the query, delete s3 bucket.
Of cource, I knew this is abnormal behaviour.... Do you have any better ideas?

I see, you're right - we have to do this 😞
Do you mind just attaching a comment explaining why we do it, so other people reading the code in the future don't need to ask the same question I just did? 🙂

radeksimko · 2017-10-27T10:56:55Z

aws/resource_aws_athena_database.go

+}
+
+func createDatabaseQueryString(databaseName string) string {
+	return fmt.Sprintf("create database %s;", databaseName)


I'm not sure I see the value in wrapping a 1-line function into another 1-line function - any reason we can't just use fmt.Sprintf("create database %s;", databaseName)?

radeksimko · 2017-10-27T10:59:40Z

aws/resource_aws_athena_database.go

+	return fmt.Sprintf("create database %s;", databaseName)
+}
+
+func checkCreateDatabaseQueryExecution(qeid string, d *schema.ResourceData, meta interface{}) error {


This function doesn't seem to need ResourceData nor all of meta - can we remove that 2nd argument and turn the last one into *athena.Athena and just pass in meta.(*AWSClient).athenaconn?

Also I think it's pretty much a copy of checkDropDatabaseQueryExecution so we can make a single function and reuse it instead, IMO.

Yeah, the main logic is the same as checkDropDatabaseQueryExecution. But I'd like to change error message.
I'm planning to apply executeAndExpectNoRowsWhenCreate and executeAndExpectNoRowsWhenDrop to each.
Do executeAndExpectNoRowsWhenCreate suit the name rules in terraform?

radeksimko · 2017-10-27T11:02:36Z

aws/resource_aws_athena_database.go

+	return fmt.Sprintf("drop database %s;", databaseName)
+}
+
+func checkDropDatabaseQueryExecution(qeid string, d *schema.ResourceData, meta interface{}) error {


I think better name for this function would be something like executeAndExpectNoRows or something like that - there's no logic related to database dropping (other than error message), so the name shouldn't be pretending there is.

radeksimko · 2017-10-27T11:09:45Z

aws/resource_aws_athena_database.go

+	return fmt.Sprint("show databases;")
+}
+
+func checkShowDatabaseQueryExecution(qeid string, d *schema.ResourceData, meta interface{}) error {


I think better name for this function would be something like executeAndExpectMatchingRow or something like that - there's no logic related to showing DB (other than error message & variable name), so the name shouldn't be pretending there is.

radeksimko · 2017-10-27T11:10:07Z

aws/resource_aws_athena_database.go

+				found = true
+			}
+		}
+	}


Can't we just always expect 1 row to be returned and error out if that's not the case? Instead of nesting the for loops here?

I depend on that show databases; won't return the same name databases.
So if the query return multiple same database, this function doesn't error out

I agree that this operation isn't clear 😕

I can't think of any situation where show database X would return more than 1 row (the DB name). Is there such situation? It's no big deal, just that it seems unnecessary.

atsushi-ishibashi · 2017-10-28T02:31:14Z

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAthenaDatabase_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAthenaDatabase_basic -timeout 120m
=== RUN   TestAccAWSAthenaDatabase_basic
--- PASS: TestAccAWSAthenaDatabase_basic (102.17s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	102.212s

atsushi-ishibashi · 2017-11-09T05:14:14Z

@radeksimko It'd be greate if you could give additional feedback when you have time 🙇

radeksimko

I left you some additional comments, it's getting closer to the finish line, i.e. mergeable state.
Let me know if you need any clarification.

radeksimko · 2017-10-28T07:21:50Z

aws/resource_aws_athena_database.go

+				found = true
+			}
+		}
+	}


I can't think of any situation where show database X would return more than 1 row (the DB name). Is there such situation? It's no big deal, just that it seems unnecessary.

radeksimko · 2017-10-28T07:24:28Z

aws/resource_aws_athena_database_test.go

+	})
+}
+
+func testAccCheckAWSAthenaDatabaseDestroy(s *terraform.State) error {


I see, you're right - we have to do this 😞
Do you mind just attaching a comment explaining why we do it, so other people reading the code in the future don't need to ask the same question I just did? 🙂

radeksimko · 2017-10-28T07:25:54Z

aws/resource_aws_athena_database_test.go

+		if err != nil {
+			return err
+		}
+		defer func() {


What is the reason for the empty deferred function here?

radeksimko · 2017-10-28T07:34:06Z

aws/resource_aws_athena_database.go

+			return out, athena.QueryExecutionStateCancelled, fmt.Errorf("[Error] QueryExecution Canceled")
+		default:
+			return out, *out.QueryExecution.Status.State, fmt.Errorf("[Error] Unexpected QueryExecution State: %s", *out.QueryExecution.Status.State)
+		}


I think the whole switch block above can be shortened to something like this:

return out, *out.QueryExecution.Status.State, nil

the retry helper has its own ways to detect unexpected states and return some reasonable error messages.

One question I have - is there a way to find out why a query failed or why it was cancelled? Then we could have a special case for either of these states and expose the reason. If we have no reason to expose we're adding no more value, so there's no point of treating these states separately.

radeksimko · 2017-10-28T07:36:37Z

aws/resource_aws_athena_database.go

+	return nil
+}
+
+func executeAndExpectMatchingRow(qeid string, d *schema.ResourceData, conn *athena.Athena) error {


I think we don't need the whole ResourceData here - how about passing value string instead?

That will also enable us to reuse that function in the acceptance test. 😉

ghost · 2020-04-10T17:17:33Z

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. Thanks!

Make files

152df05

radeksimko added the new-resource Introduces a new resource. label Oct 17, 2017

atsushi-ishibashi added 3 commits October 21, 2017 00:45

Merge branch 'master' of https://github.com/terraform-providers/terra…

654c1f4

…form-provider-aws into resouce_aws_athena_database

wip

0c513f3

implement CRD methods

2cd4c28

make docs

d8c71cb

atsushi-ishibashi changed the title ~~[WIP]New Resource: aws_athena_database~~ New Resource: aws_athena_database Oct 24, 2017

radeksimko reviewed Oct 27, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 27, 2017

Reflect reviews

aa839dc

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 28, 2017

radeksimko reviewed Nov 9, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Nov 9, 2017

atsushi-ishibashi added 3 commits November 9, 2017 18:12

Reflect 2nd reviews

fad47f5

Merge branch 'master' into resouce_aws_athena_database

0400a4a

Merge branch 'master' into resouce_aws_athena_database

b3cf14c

radeksimko approved these changes Nov 9, 2017

View reviewed changes

radeksimko merged commit 76052ff into hashicorp:master Nov 9, 2017

atsushi-ishibashi deleted the resouce_aws_athena_database branch December 13, 2017 15:16

ghost locked and limited conversation to collaborators Apr 10, 2020

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_athena_database #1922

New Resource: aws_athena_database #1922

atsushi-ishibashi commented Oct 17, 2017

atsushi-ishibashi commented Oct 23, 2017

atsushi-ishibashi commented Oct 23, 2017

atsushi-ishibashi commented Oct 27, 2017 •

edited

Loading

radeksimko left a comment

radeksimko Oct 27, 2017

radeksimko Oct 27, 2017

atsushi-ishibashi Oct 28, 2017

radeksimko Oct 27, 2017

radeksimko Oct 27, 2017

radeksimko Oct 27, 2017

atsushi-ishibashi Oct 28, 2017 •

edited

Loading

radeksimko Oct 28, 2017

radeksimko Oct 27, 2017

radeksimko Oct 27, 2017

radeksimko Oct 27, 2017

atsushi-ishibashi Oct 28, 2017

radeksimko Oct 27, 2017

radeksimko Oct 27, 2017

radeksimko Oct 27, 2017

atsushi-ishibashi Oct 28, 2017

atsushi-ishibashi Oct 28, 2017

radeksimko Oct 28, 2017

atsushi-ishibashi commented Oct 28, 2017

atsushi-ishibashi commented Nov 9, 2017

radeksimko left a comment •

edited

Loading

radeksimko Oct 28, 2017

radeksimko Oct 28, 2017

radeksimko Oct 28, 2017

radeksimko Oct 28, 2017

radeksimko Oct 28, 2017

ghost commented Apr 10, 2020

New Resource: aws_athena_database #1922

New Resource: aws_athena_database #1922

Conversation

atsushi-ishibashi commented Oct 17, 2017

atsushi-ishibashi commented Oct 23, 2017

atsushi-ishibashi commented Oct 23, 2017

atsushi-ishibashi commented Oct 27, 2017 • edited Loading

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Oct 28, 2017

atsushi-ishibashi commented Nov 9, 2017

radeksimko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 10, 2020

atsushi-ishibashi commented Oct 27, 2017 •

edited

Loading

atsushi-ishibashi Oct 28, 2017 •

edited

Loading

radeksimko left a comment •

edited

Loading