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.

Sign up for GitHub

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

Jump to bottom

New resource: service discovery http namespace #6864

Merged

bflad merged 5 commits into hashicorp:master from saravanan30erd:issue-6783

Dec 19, 2018

Contributor

saravanan30erd commented Dec 15, 2018

Fixes #6783

Output from acceptance testing:

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

saravanan30erd added 3 commits

December 15, 2018 15:01


          create resource service_discovery_http_namespace

5c42e3e


          add acceptance test for service discovery http namespace

133ff2a


          updated documentation for http namespace

65786f2

ghost added size/L provider service/servicediscovery tests documentation labels

bflad added the new-resource label

bflad reviewed

View reviewed changes

Contributor

bflad left a comment

Thanks so much for submitting this @saravanan30erd! A few little things and it should be good to go. Please let us know if you have any questions or do not have time to implement the feedback. 😄

aws/resource_aws_service_discovery_http_namespace.go Outdated

+              	input := &servicediscovery.CreateHttpNamespaceInput{
+              		Name:             aws.String(name),
+              		CreatorRequestId: aws.String(requestId),

Contributor

bflad Dec 19, 2018

Since it doesn't look like we need any real value for this ID since we don't save it anywhere and according to the SDK docs:

// CreatorRequestId can be any unique string, for example, a date/time stamp.

Should we remove all the logic above and simplify this?

Suggested change

      
            		CreatorRequestId: aws.String(requestId),
          
            		CreatorRequestId: aws.String(resource.UniqueId()),

aws/resource_aws_service_discovery_http_namespace.go Outdated

+              	resp, err := conn.CreateHttpNamespace(input)
+              	if err != nil {
+              		return err

Contributor

bflad Dec 19, 2018

Nit: We should provide context about when this error occurred for operators and code maintainers, e.g.

Suggested change

      
            		return err
          
            		return fmt.Errorf("error creating Service Discovery HTTP Namespace (%s): %s", name, err)

aws/resource_aws_service_discovery_http_namespace.go Outdated

+              	opresp, err := stateConf.WaitForState()
+              	if err != nil {
+              		return err

Contributor

bflad Dec 19, 2018

Nit: We should provide context about when this error occurred for operators and code maintainers, e.g.

Suggested change

      
            		return err
          
            		return fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) creation: %s", name, err)

aws/resource_aws_service_discovery_http_namespace.go Outdated

+              			d.SetId("")
+              			return nil
+              		}
+              		return err

Contributor

bflad Dec 19, 2018

Nit: We should provide context about when this error occurred for operators and code maintainers, e.g.

Suggested change

      
            		return err
          
            		return fmt.Errorf("error reading Service Discovery HTTP Namespace (%s): %s", d.Id(), err)

aws/resource_aws_service_discovery_http_namespace.go Outdated

+              	resp, err := conn.DeleteNamespace(input)
+              	if err != nil {
+              		return err

Contributor

bflad Dec 19, 2018

Nit: We should provide context about when this error occurred for operators and code maintainers, e.g.

Suggested change

      
            		return err
          
            		return fmt.Errorf("error deleting Service Discovery HTTP Namespace (%s): %s", d.Id(), err)

aws/resource_aws_service_discovery_http_namespace.go Outdated

+              	_, err = stateConf.WaitForState()
+              	if err != nil {
+              		return err

Contributor

bflad Dec 19, 2018

Nit: We should provide context about when this error occurred for operators and code maintainers, e.g.

Suggested change

      
            		return err
          
            		return fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) deletion: %s", d.Id(), err)

aws/resource_aws_service_discovery_http_namespace.go

+              		return err
+              	}
+              	d.SetId(*opresp.(*servicediscovery.GetOperationOutput).Operation.Targets["NAMESPACE"])

Contributor

bflad Dec 19, 2018

Normally we'd ask for d.SetId() to be right after the creation and before the waiter to prevent creating potentially orphaned resources, but since the API doesn't return the namespace ID until you read the operation in the waiter, this is okay. 👍

aws/resource_aws_service_discovery_http_namespace_test.go Outdated

+              func TestAccAWSServiceDiscoveryHttpNamespace_basic(t *testing.T) {
+              	resourceName := "aws_service_discovery_http_namespace.test"
+              	rName := "terraformtesting"

Contributor

bflad Dec 19, 2018

It'd be good to randomize this now, before additional parallel tests are added. 👍

Suggested change

      
            	rName := "terraformtesting"
          
            	rName := acctest.RandomWithPrefix("tf-acc-test")

aws/resource_aws_service_discovery_http_namespace_test.go Outdated

+              		_, err := conn.GetNamespace(input)
+              		if err != nil {
+              			if isAWSErr(err, servicediscovery.ErrCodeNamespaceNotFound, "") {
+              				return nil

Contributor

bflad Dec 19, 2018

This return nil should be replaced with continue so the for loop can check other aws_service_discovery_http_namespace resources for deletion if/when a test configuration contains multiple. 👍

Suggested change

      
            				return nil
          
            				continue

aws/resource_aws_service_discovery_http_namespace_test.go

+              	return fmt.Sprintf(`
+              resource "aws_service_discovery_http_namespace" "test" {
+                name = %q
+                description = "test"

Contributor

bflad Dec 19, 2018

Nit: While its likely handled properly in the resource code already, it'd be nice if description was removed in the basic test configuration and tested separately since it is an optional argument.

Contributor

bflad Dec 19, 2018

Sorry I probably should have been more clear what I meant. While splitting out the description test was the first step, the next steps were removing this line here in testAccServiceDiscoveryHttpNamespaceConfig() so TestAccAWSServiceDiscoveryHttpNamespace_basic() doesn't add Description to the CreateHttpNamespace call, then creating a separate configuration function for usage in the description test:

func testAccServiceDiscoveryHttpNamespaceConfigDescription(rName, description string) string {
	return fmt.Sprintf(`
resource "aws_service_discovery_http_namespace" "test" {
  description = %q
  name        = %q
}
`, description, rName)
}

That's my bad. I'll make this adjustment on merge rather than bothering you about it.

bflad added the waiting-response label

saravanan30erd added 2 commits

December 19, 2018 09:56


          fixed error handling

13335cd


          updated the acceptance tests in service_sidcovery

c2b0315

ghost added size/XL and removed size/L labels

Contributor Author

saravanan30erd commented Dec 19, 2018

@bflad Done all the changes

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

ghost removed the waiting-response label

bflad approved these changes

View reviewed changes

Contributor

bflad left a comment

LGTM, thanks @saravanan30erd! 🚀

--- PASS: TestAccAWSServiceDiscoveryHttpNamespace_basic (89.74s)
--- PASS: TestAccAWSServiceDiscoveryHttpNamespace_checkDescription (89.78s)

aws/resource_aws_service_discovery_http_namespace_test.go

+              	return fmt.Sprintf(`
+              resource "aws_service_discovery_http_namespace" "test" {
+                name = %q
+                description = "test"

Contributor

bflad Dec 19, 2018

Sorry I probably should have been more clear what I meant. While splitting out the description test was the first step, the next steps were removing this line here in testAccServiceDiscoveryHttpNamespaceConfig() so TestAccAWSServiceDiscoveryHttpNamespace_basic() doesn't add Description to the CreateHttpNamespace call, then creating a separate configuration function for usage in the description test:

func testAccServiceDiscoveryHttpNamespaceConfigDescription(rName, description string) string {
	return fmt.Sprintf(`
resource "aws_service_discovery_http_namespace" "test" {
  description = %q
  name        = %q
}
`, description, rName)
}

That's my bad. I'll make this adjustment on merge rather than bothering you about it.

bflad added this to the v1.53.0 milestone

bflad merged commit c2b0315 into hashicorp:master

bflad added a commit that referenced this pull request


          Update CHANGELOG for #6864

af59dd2

Contributor

bflad commented Dec 20, 2018

This has been released in version 1.53.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

namliz commented Dec 27, 2018 •

edited

Loading

@bflad I'm glad this was merged but it is missing functionality or is slightly wrong in how it does things.

Opened #7016.

ghost commented Apr 1, 2020

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!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation new-resource provider service/servicediscovery size/XL tests