-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
New Resource: AWS Glue Catalog Database #2175
Conversation
Found the `resource.TestCheckResourceAttr` handles functionality I implemented manually. Have removed this custom code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI the maintainers usually prefer to treat the vendor changes separately since they tend to be a fast moving target (not sure if this is specifically mentioned anywhere). This is to keep the version synchronized everywhere, prevent merge conflicts, and reduce PR changes.
# Conflicts: # vendor/vendor.json
# Conflicts: # aws/config.go
|
Is there anything else which is needed to get this PR in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @drewsonne -- thanks for your work here! I left you some initial feedback on your PR. Please take a look and let me know if you have any questions.
Provides a Glue Catalog Database. | ||
--- | ||
|
||
# aws\_glue\_catalog\_database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: (likely copypasta) antislashes in the documentation are no longer needed
|
||
## Attributes Reference | ||
|
||
The following attributes are exported: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nitpicks:
- (likely copypasta) this should read
The following additional attributes are exported
- We should list what the
id
attribute is here
|
||
## Import | ||
|
||
Glue Catalog Databasess can be imported using the `name`, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Databases
glueconn := meta.(*AWSClient).glueconn | ||
name := d.Get("name").(string) | ||
|
||
input := &glue.CreateDatabaseInput{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this form, it does not support this input parameter:
// The ID of the Data Catalog in which to create the database. If none is supplied,
// the AWS account ID is used by default.
CatalogId *string `min:"1" type:"string"`
I am wondering if we need to or should support this.
return fmt.Errorf("Error creating Catalogue Database: %s", err) | ||
} | ||
|
||
d.SetId(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the above about potentially supporting separate catalogs in an AWS account, does this resource ID need to support the CatalogId
as well? e.g. fmt.Sprintf("%s:%s", meta.(*AWSClient).accountid, name)
}, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"description": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(likely copypasta) since Go 1.7 you do not need to duplicate the &schema.Schema
for each of the attributes
Elem: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"create_time": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For most or all AWS resources we do not export create/update time attributes like these. Is there a specific use case for dealing with this attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no specific use case, my motivation was simply to expose the Glue API for the Database object with as little logic as possible. If the convention within the AWS resources in terraform is not to expose this, I will remove it. Can you just confirm that hiding the CreateTime
attribute would be part of the convention within the provider?
} | ||
if _, err := conn.GetDatabase(input); err != nil { | ||
//Verify the error is what we want | ||
if ae, ok := err.(awserr.Error); ok && ae.Code() == "EntityNotFoundException" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two notes about this:
- We have a helper function which can let you remove the
awserr
import from this file - I would recommend using the SDK constant here
isAWSErr(err, glue.ErrCodeEntityNotFoundException, "")
"github.com/hashicorp/terraform/terraform" | ||
) | ||
|
||
func TestAccAWSGlueCatalogDatabase_basic(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a second resource.TestStep
that updates the attributes and ensures the updates propagate properly to AWS/Terraform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have attempted to do this, but I struggling to get it working.
When I add a second step with a modified description
field, I get:
testing.go:503: Step 0 error: After applying this step, the plan was not empty:
DIFF:
DESTROY/CREATE: aws_glue_catalog_database.test
catalog_id: "440474553311" => "" (forces new resource)
description: "A test catalog from terraform" => "A test catalog from terraform"
location_uri: "my-location" => "my-location"
name: "my_test_catalog_database_6335041086110733121" => "my_test_catalog_database_6335041086110733121"
parameters.%: "3" => "3"
parameters.param1: "value1" => "value1"
parameters.param2: "1" => "1"
parameters.param3: "50" => "50"
STATE:
aws_glue_catalog_database.test:
ID = 440474553311:my_test_catalog_database_6335041086110733121
catalog_id = 440474553311
description = A test catalog from terraform
location_uri = my-location
name = my_test_catalog_database_6335041086110733121
parameters.% = 3
parameters.param1 = value1
parameters.param2 = 1
parameters.param3 = 50
When I take the second step out, it works, so I am not sure why it's saying the plan was not empty at the end if it succeeded with the first step. If there's only one step, does it not test for an empty plan?
I've had a look at https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_ecs_service_test.go to get an idea of a config change test, but I can't see why mine fails and that one works. @bflad , are you able to shed any light on this? I'm keen to get this test working, as it's a problem I ran across in my own provider. That is, how to test diffs successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try adding Computed: true
to the optional catalog_id
attribute in the schema. This should tell Terraform that its value is allowed to be set from the provider instead of just the configuration. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr Computed: true
should be set on any attributes that AWS might return to be set in the state that aren't required by writing Terraform configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks! I was under the impression Computed: true
was only for attributes which are computed, and not attributes which could be computed. This is working on my integration tests now.
$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueCatalogDatabase_*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSGlueCatalogDatabase_* -timeout 120m
=== RUN TestAccAWSGlueCatalogDatabaseVault_importBasic
--- PASS: TestAccAWSGlueCatalogDatabaseVault_importBasic (22.05s)
=== RUN TestAccAWSGlueCatalogDatabase_basic
--- PASS: TestAccAWSGlueCatalogDatabase_basic (33.43s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 55.511s
Amazing thank you! I will have a work through those when I next get a chance. I based this off the glacier resource, so that's where the conventions came from. |
Rather than passing in a very generic variable `meta` of type `interface{}`, require a specific type (string) for the account id which is being passed in.
# Conflicts: # aws/config.go # aws/provider.go # website/aws.erb
|
Also... if I wanted to add a list of things I have learned in this PR about how one goes about building a resource (code style, terraform-provider-aws idiomatic process, what type of tests, when to use |
https://github.com/terraform-providers/terraform-provider-aws/blob/master/.github/CONTRIBUTING.md is probably the best place for such things. |
Hope that other resources will be added too. 👍 Thanks for the great PR. |
Sigh. Ok, the glue module in vendor has been deleted from the master branch, so I'm going to go through the vendor PR's and find out where it went. |
Looks like it was deleted during an update to terraform 0.11.2. @bflad @radeksimko does this need a separate PR to pull this dependency back into master, or can I put it in this PR? |
Ahh, sorry for this. Looks like we were a bit too excited in #2722 about removal of some "unused" packages... I can track down the rest of missing packages - feel free to send a PR for |
@radeksimko PR in #2933 |
@drewsonne merged - feel free to rebase. |
@radeksimko Integration tests pass.
So, @radeksimko, we think this is good to go now? :-) |
Wee! Thanks @radeksimko! Will get another one for glue in soonish |
This has been released in terraform-provider-aws version 1.7.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Implemented the AWS Glue Catalog database resource as described here http://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-catalog-databases.html to address #1416.
This is my first PR here so I want to keep it simple and only implement a single resource and will implement other resources for AWS glue once I've done the process once