Only accept lowercase names for aws_glue_catalog_database and aws_glue_catalog_table #8288

jukie · 2019-04-11T23:41:51Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Changes proposed in this pull request:

Validate database name provided is only comprised of lowercase alphanumeric characters, underscores, and hyphens

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueCatalogDatabase_validation'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSGlueCatalogDatabase_validation -timeout 120m
=== RUN   TestAccAWSGlueCatalogDatabase_validation
=== PAUSE TestAccAWSGlueCatalogDatabase_validation
=== CONT  TestAccAWSGlueCatalogDatabase_validation
--- PASS: TestAccAWSGlueCatalogDatabase_validation (1.61s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1.650s
$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueCatalogTable_validation'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSGlueCatalogTable_validation -timeout 120m
=== RUN   TestAccAWSGlueCatalogTable_validation
=== PAUSE TestAccAWSGlueCatalogTable_validation
=== CONT  TestAccAWSGlueCatalogTable_validation
--- PASS: TestAccAWSGlueCatalogTable_validation (0.75s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.793s

aws/resource_aws_glue_catalog_database_test.go

…n examples

…db name

jukie · 2019-04-14T22:24:13Z

Updated acceptance test results:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueCatalogTable_validation' && make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueCatalogDatabase_validation'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSGlueCatalogTable_validation -timeout 120m
=== RUN   TestAccAWSGlueCatalogTable_validation
=== PAUSE TestAccAWSGlueCatalogTable_validation
=== CONT  TestAccAWSGlueCatalogTable_validation
--- PASS: TestAccAWSGlueCatalogTable_validation (4.34s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	4.398s
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSGlueCatalogDatabase_validation -timeout 120m
=== RUN   TestAccAWSGlueCatalogDatabase_validation
=== PAUSE TestAccAWSGlueCatalogDatabase_validation
=== CONT  TestAccAWSGlueCatalogDatabase_validation
--- PASS: TestAccAWSGlueCatalogDatabase_validation (1.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	(cached)

jukie · 2019-04-15T17:21:37Z

This is ready for a follow-up review when there's time.

jukie · 2019-05-15T12:25:02Z

@charlyx is this ready to merge?

jukie · 2019-06-10T03:14:28Z

I see this pr was approved a while ago, is it ready to merge?

carinadigital · 2019-09-24T17:18:41Z

@jukie Are going to bring this PR up to date? I'm happy to help.

carinadigital · 2019-09-24T17:20:48Z

website/docs/r/glue_catalog_database.html.markdown

@@ -14,7 +14,7 @@ Provides a Glue Catalog Database Resource. You can refer to the [Glue Developer

 ```hcl
 resource "aws_glue_catalog_database" "aws_glue_catalog_database" {
-  name = "MyCatalogDatabase"
+  name = "my_datalog_database"


carinadigital · 2019-09-24T17:21:36Z

website/docs/r/glue_catalog_table.html.markdown

-  name          = "MyCatalogTable"
-  database_name = "MyCatalogDatabase"
+  name          = "my_catalog_table"
+  database_name = "my_datalog_database"


And same one here

carinadigital · 2019-09-24T17:22:03Z

website/docs/r/glue_catalog_table.html.markdown

-  name          = "MyCatalogTable"
-  database_name = "MyCatalogDatabase"
+  name          = "my_datalog_table"
+  database_name = "my_datalog_database"


carinadigital · 2019-09-24T17:41:04Z

aws/validators.go

+	if len(value) > 255 {
+		errors = append(errors, fmt.Errorf("%q cannot be more than 255 characters long", k))
+	}
+	return


Maybe also validate the whole string against a validity regex to stop special characters getting through. e.g.

^[a-z0-9_-]*$

or even better use AWS's
https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-catalog-databases.html
https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-common.html#aws-glue-api-regex-oneLine

[\u0020-\uD7FF\uE000-\uFFFD\uD800\uDC00-\uDBFF\uDFFF\t]*

I don't think that regex is valid, \uDC00-\uDBFF is an invalid range. I only have minimal experience using unicode expressions but as long as I've converted to go notation correctly I think we've found a bug.
https://play.golang.org/p/jhV8pEEjPrT

Additionally, special characters are in fact allowed in both the table and database names. Technically, Uppercase is allowed in the api call as well but it's just converted to lowercase after.
This proposal for only accepting lowercase in terraform is because otherwise terraform would always attempt to reapply changes. I think my validator works but if you'd like me to make changes I can.

No, I think your right 👍

jukie · 2019-09-27T22:22:19Z

Thanks, I'll rebase and apply your suggested changes

carinadigital · 2019-09-29T09:26:43Z

Let's hope someone from hashicorp will pick this up now :-)

jukie · 2019-09-30T00:55:04Z

Thanks @carinadigital!
#8384 is another one that's been open for awhile if you have time to take a look.

bflad

Thanks for submitting this, @jukie. I left some minor feedback items below and marked this for our upcoming 3.0.0 version (likely in the next month or so) since technically it can break existing configurations that might be working around the perpetual difference with ignore_changes. Please reach out if you have any questions or do not have time to implement the feedback items.

bflad · 2019-10-11T22:59:38Z

aws/resource_aws_glue_catalog_database_test.go

@@ -12,6 +13,54 @@ import (
 	"github.com/hashicorp/terraform/terraform"
 )

+func TestAccAWSGlueCatalogDatabase_importBasic(t *testing.T) {


We're in the process of removing import-only testing (#8944). Please add the ImportState testing to existing tests if its not present, although TestAccAWSGlueCatalogDatabase_full on master looks like it includes it already.

bflad · 2019-10-11T23:00:54Z

aws/resource_aws_glue_catalog_database_test.go

@@ -208,6 +257,21 @@ resource "aws_glue_catalog_database" "test" {
 `, rInt, desc)
 }

+func testAccGlueCatalogDatabase_validation(rName string) string {


For clarity, it might be better if this was named against the attribute its testing:

Suggested change

func testAccGlueCatalogDatabase_validation(rName string) string {

func testAccGlueCatalogDatabaseConfigName(rName string) string {

bflad · 2019-10-11T23:01:41Z

aws/resource_aws_glue_catalog_database_test.go

+	return fmt.Sprintf(`
+resource "aws_glue_catalog_database" "test" {
+  name = "%s"
+  description = "validation test"


Given the rest of the arguments are optional and we're only looking to test the name attribute, I'd suggest removing the other arguments from the test configuration.

tzzh · 2020-02-19T11:18:55Z

@jukie are you still working on this ? I ran into this issue as well and I am happy to help if I can.

DrFaust92 · 2020-09-23T21:06:24Z

Hey @@jukie, can you rebase?

DrFaust92 · 2020-11-05T16:57:48Z

Closing in favour of #16052

ghost · 2020-12-05T17:11:19Z

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!

jukie added 2 commits April 11, 2019 19:15

Add validations for db name casing

ab2576d

add regexp dependency

874c3d0

ghost added size/XS Managed by automation to categorize the size of a PR. service/glue Issues and PRs that pertain to the glue service. labels Apr 11, 2019

jukie mentioned this pull request Apr 11, 2019

Database name in aws_glue_catalog_database and aws_glue_catalog_table should be lowercase #7958

Closed

jukie and others added 2 commits April 11, 2019 22:12

Move validate function to aws/validators.go

d7e87b4

add test case for validation error

10dfffe

jukie force-pushed the glue-case-check branch from d9265c7 to 10dfffe Compare April 12, 2019 02:13

ghost added size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels Apr 12, 2019

jukie added 2 commits April 12, 2019 09:18

fix regex - all special characters allowed

a42300f

add testAcc functions for catalogtable

6047537

charlyx reviewed Apr 14, 2019

View reviewed changes

aws/resource_aws_glue_catalog_database_test.go Show resolved Hide resolved

jukie added 3 commits April 14, 2019 18:17

Update webiste docs to use lowercase table names and database names i…

9c73bad

…n examples

add validation tests for table name, add test case for valid table & …

6cacfbb

…db name

add test case for valid database name

96ab7a9

ghost added size/L Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. and removed size/M Managed by automation to categorize the size of a PR. labels Apr 14, 2019

charlyx approved these changes Apr 19, 2019

View reviewed changes

jukie mentioned this pull request May 30, 2019

Place note in aws_glue_catalog_database that name is case-sensitive #8814

Closed

aeschright requested a review from a team June 26, 2019 16:42

carinadigital suggested changes Sep 24, 2019

View reviewed changes

jukie added 2 commits September 27, 2019 18:31

rebasing

2a54335

Fix typos in docs

0e6a4f5

carinadigital approved these changes Sep 29, 2019

View reviewed changes

jukie mentioned this pull request Oct 11, 2019

Cleanup randomString() #10478

Merged

jukie removed the request for review from a team October 11, 2019 21:15

bflad added this to the v3.0.0 milestone Oct 11, 2019

bflad suggested changes Oct 11, 2019

View reviewed changes

bflad mentioned this pull request Oct 11, 2019

Glue Crawler database name should be lower case #10252

Closed

bflad mentioned this pull request Nov 20, 2019

Glue Database: Error: Provider produced inconsistent final plan #10947

Closed

breathingdust removed this from the v3.0.0 milestone May 19, 2020

jukie requested a review from a team July 21, 2020 18:26

DrFaust92 closed this Nov 5, 2020

ghost locked as resolved and limited conversation to collaborators Dec 5, 2020

justinretzolk added the partner Contribution from a partner. label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only accept lowercase names for aws_glue_catalog_database and aws_glue_catalog_table #8288

Only accept lowercase names for aws_glue_catalog_database and aws_glue_catalog_table #8288

jukie commented Apr 11, 2019 •

edited

Loading

jukie commented Apr 14, 2019

jukie commented Apr 15, 2019

jukie commented May 15, 2019

jukie commented Jun 10, 2019

carinadigital commented Sep 24, 2019

carinadigital Sep 24, 2019

carinadigital Sep 24, 2019

carinadigital Sep 24, 2019

carinadigital Sep 24, 2019

carinadigital Sep 24, 2019

jukie Sep 28, 2019

jukie Sep 28, 2019 •

edited

Loading

carinadigital Sep 29, 2019

jukie commented Sep 27, 2019

carinadigital commented Sep 29, 2019

jukie commented Sep 30, 2019

bflad left a comment

bflad Oct 11, 2019

bflad Oct 11, 2019

bflad Oct 11, 2019

tzzh commented Feb 19, 2020

DrFaust92 commented Sep 23, 2020

DrFaust92 commented Nov 5, 2020

ghost commented Dec 5, 2020

	func testAccGlueCatalogDatabase_validation(rName string) string {
	func testAccGlueCatalogDatabaseConfigName(rName string) string {

Only accept lowercase names for aws_glue_catalog_database and aws_glue_catalog_table #8288

Only accept lowercase names for aws_glue_catalog_database and aws_glue_catalog_table #8288

Conversation

jukie commented Apr 11, 2019 • edited Loading

Community Note

jukie commented Apr 14, 2019

jukie commented Apr 15, 2019

jukie commented May 15, 2019

jukie commented Jun 10, 2019

carinadigital commented Sep 24, 2019

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

jukie Sep 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukie commented Sep 27, 2019

carinadigital commented Sep 29, 2019

jukie commented Sep 30, 2019

bflad 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

tzzh commented Feb 19, 2020

DrFaust92 commented Sep 23, 2020

DrFaust92 commented Nov 5, 2020

ghost commented Dec 5, 2020

jukie commented Apr 11, 2019 •

edited

Loading

jukie Sep 28, 2019 •

edited

Loading