-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Changes from 18 commits
6afe6ea
4c79647
29286c5
cb12806
da164c4
b3de1ca
57a1230
19ab44c
bb596b8
4bfee75
dbfe0e7
3fc9052
5a603d6
9a5ed37
5d717d7
4ebc382
5fec001
8744d9b
a0b825d
ec0527e
5ffc1b0
d6de489
b276648
27d0cb7
e70c722
886b5ed
429d2a7
6d35a39
2975924
935fcf4
20faedc
af18cc6
e5305c3
7511705
efbc83a
b8fd125
bb60b99
482db05
d27cc24
7701582
878627a
8cdcebc
419eaa0
7d6e0dd
853efbd
f285890
46302bf
15c87e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package aws | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/hashicorp/terraform/helper/acctest" | ||
"github.com/hashicorp/terraform/helper/resource" | ||
) | ||
|
||
func TestAccAWSGlueCatalogDatabaseVault_importBasic(t *testing.T) { | ||
resourceName := "aws_glue_catalog_database.test" | ||
rInt := acctest.RandInt() | ||
|
||
resource.Test(t, resource.TestCase{ | ||
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckGlueDatabaseDestroy, | ||
Steps: []resource.TestStep{ | ||
resource.TestStep{ | ||
Config: testAccGlueCatalogDatabase_basic(rInt), | ||
}, | ||
|
||
resource.TestStep{ | ||
ResourceName: resourceName, | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
}, | ||
}, | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
package aws | ||
|
||
import ( | ||
"fmt" | ||
"github.com/aws/aws-sdk-go/aws" | ||
"github.com/aws/aws-sdk-go/service/glue" | ||
"github.com/hashicorp/terraform/helper/schema" | ||
"log" | ||
) | ||
|
||
func resourceAwsGlueCatalogDatabase() *schema.Resource { | ||
return &schema.Resource{ | ||
Create: resourceAwsGlueCatalogDatabaseCreate, | ||
Read: resourceAwsGlueCatalogDatabaseRead, | ||
Update: resourceAwsGlueCatalogDatabaseUpdate, | ||
Delete: resourceAwsGlueCatalogDatabaseDelete, | ||
|
||
Importer: &schema.ResourceImporter{ | ||
State: schema.ImportStatePassthrough, | ||
}, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"description": &schema.Schema{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
Type: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"location_uri": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"name": &schema.Schema{ | ||
Type: schema.TypeString, | ||
ForceNew: true, | ||
Required: true, | ||
}, | ||
"parameters": &schema.Schema{ | ||
Type: schema.TypeMap, | ||
Elem: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"create_time": &schema.Schema{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
Type: schema.TypeString, | ||
Computed: true, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason for setting the above 3 fields as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @radeksimko the motivation is because of the above discussion re computed flags with @bflad bflad:
I don't know the functional requirements here, and testing the functionality describe i the quote here to import would require a step in between each I'm happy to do this, but I have not seen any prior art as to how one would go about this. But also, I would like to have some sort of idea about what is expected in terms of functionality so I don't end up bouncing back and forth between two different opinions of Reviewers :-) I'm not sure who is closer to the project between yourself @radeksimko and @bflad :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will ask @bflad about this when he wakes up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just confirmed with @bflad privately that this was just a misunderstanding and we mutually agreed it should be removed from the schema here. Sorry for any confusion caused by this! After that I believe we're ready to merge it 🎉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my own understanding, aside from obvious things (like instance ids i-12345678, etc), when should the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is an expectation of Terraform users to see difference in The downside of marking something
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, there is a case above #2175 (comment) where I have a value ( I assume this is because AWS will eventually have the ability for multiple catalogues in a single account, a globally unique namespace for public catalogues, or something such. This then is a case where the user could provide This variable is currently But, I would be correct in saying that this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's right, |
||
}, | ||
} | ||
} | ||
|
||
func resourceAwsGlueCatalogDatabaseCreate(d *schema.ResourceData, meta interface{}) error { | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. In this form, it does not support this input parameter:
I am wondering if we need to or should support this. |
||
DatabaseInput: &glue.DatabaseInput{ | ||
Name: aws.String(name), | ||
}, | ||
} | ||
|
||
_, err := glueconn.CreateDatabase(input) | ||
if err != nil { | ||
return fmt.Errorf("Error creating Catalogue Database: %s", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: the service name is spelled There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah noo! I'm an Australian living in London! I think the world would be a bit better off with a generous sprinkling of vowels here and there ;-P |
||
} | ||
|
||
d.SetId(name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
return resourceAwsGlueCatalogDatabaseUpdate(d, meta) | ||
} | ||
|
||
func resourceAwsGlueCatalogDatabaseUpdate(d *schema.ResourceData, meta interface{}) error { | ||
glueconn := meta.(*AWSClient).glueconn | ||
doUpdate := false | ||
input := &glue.UpdateDatabaseInput{ | ||
DatabaseInput: &glue.DatabaseInput{ | ||
Name: aws.String(d.Id()), | ||
}, | ||
Name: aws.String(d.Id()), | ||
} | ||
|
||
if ok := d.HasChange("description"); ok { | ||
doUpdate = true | ||
input.DatabaseInput.Description = aws.String( | ||
d.Get("description").(string), | ||
) | ||
} | ||
|
||
if ok := d.HasChange("location_uri"); ok { | ||
doUpdate = true | ||
input.DatabaseInput.LocationUri = aws.String( | ||
d.Get("location_uri").(string), | ||
) | ||
} | ||
|
||
if ok := d.HasChange("parameters"); ok { | ||
doUpdate = true | ||
input.DatabaseInput.Parameters = make(map[string]*string) | ||
for key, value := range d.Get("parameters").(map[string]interface{}) { | ||
input.DatabaseInput.Parameters[key] = aws.String(value.(string)) | ||
} | ||
} | ||
|
||
if doUpdate { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick, but this is a tiny little bit more difficult to read/understand - could be simplified to |
||
if _, err := glueconn.UpdateDatabase(input); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return resourceAwsGlueCatalogDatabaseRead(d, meta) | ||
} | ||
|
||
func resourceAwsGlueCatalogDatabaseRead(d *schema.ResourceData, meta interface{}) error { | ||
glueconn := meta.(*AWSClient).glueconn | ||
|
||
input := &glue.GetDatabaseInput{ | ||
Name: aws.String(d.Id()), | ||
} | ||
|
||
out, err := glueconn.GetDatabase(input) | ||
if err != nil { | ||
return fmt.Errorf("Error reading Glue Cataloge Database: %s", err.Error()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: typo 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also do you mind adding a check for database which was removed outside of Terraform? something like this should suffice: if isAWSErr(err, glue.ErrCodeEntityNotFoundException, "") {
log.Printf("[WARN] Glue Catalog Database (%s) not found, removing from state", d.Id())
d.SetId("")
} you may also add an acceptance test verifying this behaviour, similar to e.g. https://github.com/terraform-providers/terraform-provider-aws/blob/79ed212c594fd8e848d0274cd0efc70bbf4cede8/aws/resource_aws_eip_test.go#L220-L238 but the test is not a blocker. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I mentioned above, I wish to round out the whole AWS Glue product set for terraform. For the sake of my sanity (getting a PR merged), can I put this test in a subsequent PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a test in a subsequent PR is totally fine. 😉 |
||
} | ||
|
||
d.Set("name", d.Id()) | ||
d.Set("create_time", out.Database.CreateTime) | ||
d.Set("description", out.Database.Description) | ||
d.Set("location_uri", out.Database.LocationUri) | ||
|
||
dParams := make(map[string]string) | ||
if len(out.Database.Parameters) > 0 { | ||
for key, value := range out.Database.Parameters { | ||
dParams[key] = *value | ||
} | ||
} | ||
d.Set("parameters", dParams) | ||
|
||
return nil | ||
} | ||
|
||
func resourceAwsGlueCatalogDatabaseDelete(d *schema.ResourceData, meta interface{}) error { | ||
glueconn := meta.(*AWSClient).glueconn | ||
|
||
log.Printf("[DEBUG] Glue Catalog Database: %s", d.Id()) | ||
_, err := glueconn.DeleteDatabase(&glue.DeleteDatabaseInput{ | ||
Name: aws.String(d.Id()), | ||
}) | ||
if err != nil { | ||
return fmt.Errorf("Error deleting Glue Catalog Database: %s", err.Error()) | ||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
package aws | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/aws/aws-sdk-go/aws/awserr" | ||
"github.com/aws/aws-sdk-go/service/glue" | ||
|
||
"github.com/aws/aws-sdk-go/aws" | ||
"github.com/hashicorp/terraform/helper/acctest" | ||
"github.com/hashicorp/terraform/helper/resource" | ||
"github.com/hashicorp/terraform/terraform" | ||
) | ||
|
||
func TestAccAWSGlueCatalogDatabase_basic(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add a second There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more. Try adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tl;dr There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great thanks! I was under the impression
|
||
rInt := acctest.RandInt() | ||
resource.Test(t, resource.TestCase{ | ||
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckGlueDatabaseDestroy, | ||
Steps: []resource.TestStep{ | ||
resource.TestStep{ | ||
Config: testAccGlueCatalogDatabase_basic(rInt), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckGlueCatalogDatabaseExists("aws_glue_catalog_database.test"), | ||
resource.TestCheckResourceAttr( | ||
"aws_glue_catalog_database.test", | ||
"description", | ||
"A test catalog from terraform", | ||
), | ||
resource.TestCheckResourceAttr( | ||
"aws_glue_catalog_database.test", | ||
"location_uri", | ||
"my-location", | ||
), | ||
resource.TestCheckResourceAttr( | ||
"aws_glue_catalog_database.test", | ||
"parameters.param1", | ||
"value1", | ||
), | ||
resource.TestCheckResourceAttr( | ||
"aws_glue_catalog_database.test", | ||
"parameters.param2", | ||
"1", | ||
), | ||
resource.TestCheckResourceAttr( | ||
"aws_glue_catalog_database.test", | ||
"parameters.param3", | ||
"50", | ||
), | ||
), | ||
}, | ||
}, | ||
}) | ||
} | ||
|
||
func testAccCheckGlueDatabaseDestroy(s *terraform.State) error { | ||
conn := testAccProvider.Meta().(*AWSClient).glueconn | ||
|
||
for _, rs := range s.RootModule().Resources { | ||
if rs.Type != "aws_glue_catalog_database" { | ||
continue | ||
} | ||
|
||
input := &glue.GetDatabaseInput{ | ||
Name: aws.String(rs.Primary.ID), | ||
} | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Two notes about this:
|
||
continue | ||
} | ||
|
||
return err | ||
} | ||
return fmt.Errorf("still exists") | ||
} | ||
return nil | ||
} | ||
|
||
func testAccGlueCatalogDatabase_basic(rInt int) string { | ||
return fmt.Sprintf(` | ||
resource "aws_glue_catalog_database" "test" { | ||
name = "my_test_catalog_database_%d" | ||
description = "A test catalog from terraform" | ||
location_uri = "my-location" | ||
parameters { | ||
param1 = "value1" | ||
param2 = true | ||
param3 = 50 | ||
} | ||
} | ||
`, rInt) | ||
} | ||
|
||
func testAccCheckGlueCatalogDatabaseExists(name string) resource.TestCheckFunc { | ||
return func(s *terraform.State) error { | ||
rs, ok := s.RootModule().Resources[name] | ||
if !ok { | ||
return fmt.Errorf("Not found: %s", name) | ||
} | ||
|
||
if rs.Primary.ID == "" { | ||
return fmt.Errorf("No ID is set") | ||
} | ||
|
||
glueconn := testAccProvider.Meta().(*AWSClient).glueconn | ||
out, err := glueconn.GetDatabase(&glue.GetDatabaseInput{ | ||
Name: aws.String(rs.Primary.ID), | ||
}) | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
if out.Database == nil { | ||
return fmt.Errorf("No Glue Database Found") | ||
} | ||
|
||
if *out.Database.Name != rs.Primary.ID { | ||
return fmt.Errorf("Glue Database Mismatch - existing: %q, state: %q", | ||
*out.Database, rs.Primary.ID) | ||
} | ||
|
||
return nil | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
--- | ||
layout: "aws" | ||
page_title: "AWS: aws_glue_catalog_database" | ||
sidebar_current: "docs-aws-resource-glue-catalog-database" | ||
description: |- | ||
Provides a Glue Catalog Database. | ||
--- | ||
|
||
# aws\_glue\_catalog\_database | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: (likely copypasta) antislashes in the documentation are no longer needed |
||
|
||
Provides a Glue Catalog Database Resource. You can refer to the [Glue Developer Guide](http://docs.aws.amazon.com/glue/latest/dg/populate-data-catalog.html) for a full explanation of the Glue Data Catalog functionality | ||
|
||
## Example Usage | ||
|
||
```hcl-terraform | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: We tend to use just |
||
resource "aws_glue_catalog_database" "aws_glue_catalog_database" { | ||
name = "MyCatalogDatabase" | ||
} | ||
``` | ||
|
||
## Argument Reference | ||
|
||
The following arguments are supported: | ||
|
||
* `name` - (Required) The name of the database. | ||
* `description` - (Optional) Description of the database. | ||
* `location_uri` - (Optional) The location of the database (for example, an HDFS path). | ||
* `parameters` - (Optional) A list of key-value pairs that define parameters and properties of the database. | ||
|
||
## Attributes Reference | ||
|
||
The following attributes are exported: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two nitpicks:
|
||
|
||
* `create_time` - The time at which the metadata database was created in the catalog. | ||
|
||
## 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 commentThe reason will be displayed to describe this comment to others. Learn more. Typo: |
||
|
||
``` | ||
$ terraform import aws_glue_catalog_database.database my_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: Dang, sorry I didn't notice this copypasta earlier - just needs
Vault
removed