Add force_destroy field to aws_athena_database

[betabandido]

This PR fixes #2362.

By adding a force_destroy field, it is now possible to delete a database that contains tables (by using the cascade option in the drop table query).

Additionally, error handling has been improved. When a query fails, StateChangeReason contains a message with the reason for the failure. The reason is now returned as part of the error message being printed on the console.

@atsushi-ishibashi Can you please have a look at this?

@radeksimko I can add some tests to check that destroying the database fails when force_destroy is not used, and it succeeds when the option is used. But, I would need some guidance on how to implement them. I imagine I would need to create the tables in a second test step (in the first step the creation of the database would take place) using the PreConfig hook. Is that correct?

[atsushi-ishibashi]

LGTM👍

In my opinion, StateChangeReason may be error format so it seems be better with prefix such like fmt.Errorf("[QUERY FAILED REASON] %s", *status.StateChangeReason) .
But this depends on the convention so I leave it to @radeksimko 🙇

[betabandido]

This is the current output when the error occurs:

1 error(s) occurred:

* aws_athena_database.db (destroy): 1 error(s) occurred:

* aws_athena_database.db: FAILED: Execution Error, return code 1 from
    org.apache.hadoop.hive.ql.exec.DDLTask.
    MetaException(message:Unable to drop database:
    InvalidOperationException(message:Database tf_cascade is not empty.))

[betabandido]

@Ninir Could you please let me know whether this looks good to be merged or it needs more work? I would really appreciate having this feature included in the next release, as I am using it in a project at my work :)

[radeksimko]

Hi @betabandido
thanks for raising the PR.

It looks like a reasonable feature to have. To your question:

I can add some tests to check that destroying the database fails when force_destroy is not used, and it succeeds when the option is used. But, I would need some guidance on how to implement them. I imagine I would need to create the tables in a second test step (in the first step the creation of the database would take place) using the PreConfig hook. Is that correct?

I would do it as part of a test check, e.g.

Steps: []resource.TestStep{
			{
				Config: ..., // with `force_destroy = true`
				Check: resource.ComposeTestCheckFunc(
					testAccAthenaDatabaseCreateTables(),

Let me know if you need any further help with those tests.

In my opinion, StateChangeReason may be error format so it seems be better with prefix such like fmt.Errorf("[QUERY FAILED REASON] %s", *status.StateChangeReason) .

@atsushi-ishibashi I agree with you here that some extra context would be certainly helpful. I don't think it should be there as prefix in square brackets though. That format is usually used for levels of log messages, e.g. DEBUG, INFO, WARN etc. We can do something as simple as fmt.Errorf("Reason: %s", *status.StateChangeReason).

[betabandido]

@radeksimko I added the tests and made some other minor improvements :)

[betabandido]

Is there anything left to do before merging this PR?

[radeksimko]

LGTM, thanks.

[ghost]

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!