Add support aws worklink website certificate authority association resource #7459

teraken0509 · 2019-02-06T10:58:10Z

Changes proposed in this pull request:

Add aws_worklink_website_certificate_authority_association resource

Output from acceptance testing:

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

bflad

Thanks so much @kterada0509! A few little things and this should be good.

Maintainer note: Its a shame the provider resource map change will affect other new resource/data source PRs and that other new resource data source PRs merged in before this will break this one. 😓 I think we have reasons to keep it a static map for other tooling that reads the provider information, but maybe worth reconsidering splitting it up (e.g. per service) so string length changes like this don't cause this problem.

bflad · 2019-02-06T20:04:21Z

aws/resource_aws_worklink_website_certificate_authority_association.go

+		return fmt.Errorf("Error creating WorkLink Website Certificate Authority Association: %s", err)
+	}
+
+	d.SetId(fmt.Sprintf("%s/%s", aws.StringValue(resp.WebsiteCaId), d.Get("fleet_arn").(string)))


Two things here:

I'm not sure if we note this in our contributing documentation anywhere (😅), but by convention across the Terraform AWS provider we prefer to set resource IDs (and therefore their import IDs) where the container ID is first, then the contained ID. So in this case WorkLink Fleet ARN then WorkLink Website CA ID. 👍

We may want to consider a different separator, e.g. a comma, since the ARN involved contains slashes

Suggested change

d.SetId(fmt.Sprintf("%s/%s", aws.StringValue(resp.WebsiteCaId), d.Get("fleet_arn").(string)))

d.SetId(fmt.Sprintf("%s,%s", d.Get("fleet_arn").(string), aws.StringValue(resp.WebsiteCaId)))

Thanks. I fixed to given format.

bflad · 2019-02-06T20:09:04Z

aws/resource_aws_worklink_website_certificate_authority_association.go

+			if isAWSErr(err, worklink.ErrCodeResourceNotFoundException, "") {
+				return emptyResp, "DELETED", nil
+			}
+		}


If the error is not worklink.ErrCodeResourceNotFoundException, we should return the error.

Suggested change

}

return nil, "", err

}

For what its worth, we have been slowly trying to move away from the convention of using if isAWSErr() conditionals inside the if err != nil conditionals to make this type of coding mistake more obvious, e.g. in this case

if isAWSErr(err, worklink.ErrCodeResourceNotFoundException, "") { return emptyResp, "DELETED", nil } if err != nil { return nil, "", err }

Its a bummer this cannot be caught by linting as its a logic problem. 😄

bflad · 2019-02-06T20:11:58Z

website/docs/r/worklink_website_certificate_authority_association.html.markdown

+
+resource "aws_worklink_website_certificate_authority_association" "test" {
+	fleet_arn	= "${aws_worklink_fleet.test.arn}"
+	certificate = "${file("test-fixtures/worklink-website-certificate-authority-association.pem")}"


Nit: We may want to remove the test-fixtures part of this for the example to reduce any confusion 👍

certificate = "${file("certificate.pem")}"

bflad · 2019-02-06T20:12:48Z

website/docs/r/worklink_website_certificate_authority_association.html.markdown

+
+## Import
+
+WorkLink Website Certificate Authority can be imported using `WEBSITE-CA-ID/FLEET-ARN`, e.g.


Just noting here that when the ID is swapped around, this documentation will also need an update 👍

teraken0509 · 2019-02-07T01:36:38Z

@bflad
I all fixed your review comment and tests are passed.

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

…resource

…on resource

This reverts commit 9e34d984560ff4ec526be7636a06124069bc1b5a.

bflad

LGTM, thanks @kterada0509! 🚀 Apologies in the delay getting back to this and appreciate the recent rebase work.

Output from acceptance testing:

--- PASS: TestAccAWSWorkLinkWorkLinkWebsiteCertificateAuthorityAssociation_Basic (91.33s)
--- PASS: TestAccAWSWorkLinkWorkLinkWebsiteCertificateAuthorityAssociation_Disappears (99.38s)
--- PASS: TestAccAWSWorkLinkWorkLinkWebsiteCertificateAuthorityAssociation_DisplayName (104.30s)

bflad · 2019-04-11T01:08:57Z

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

ghost · 2020-03-05T14:52:53Z

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!

teraken0509 changed the title ~~[WIP] Add support aws worklink website certificate authority association resource~~ Add support aws worklink website certificate authority association resource Feb 6, 2019

ewbankkit mentioned this pull request Feb 6, 2019

Feature request: Support Amazon WorkLink #7330

Closed

2 tasks

bflad added the new-resource Introduces a new resource. label Feb 6, 2019

bflad reviewed Feb 6, 2019

View reviewed changes

teraken0509 force-pushed the feature/add-support-aws_worklink_website_certificate_authority_association-resource branch from 924ee80 to dc2d03e Compare February 7, 2019 01:34

teraken0509 force-pushed the feature/add-support-aws_worklink_website_certificate_authority_association-resource branch 5 times, most recently from aa848ba to e60e820 Compare February 13, 2019 02:58

teraken0509 force-pushed the feature/add-support-aws_worklink_website_certificate_authority_association-resource branch 2 times, most recently from 8041594 to dedac6f Compare February 22, 2019 07:56

teraken0509 force-pushed the feature/add-support-aws_worklink_website_certificate_authority_association-resource branch from dedac6f to da8e911 Compare March 7, 2019 06:56

teraken0509 force-pushed the feature/add-support-aws_worklink_website_certificate_authority_association-resource branch from da8e911 to 1db47d3 Compare March 15, 2019 04:29

bflad added the service/worklink Issues and PRs that pertain to the worklink service. label Apr 2, 2019

teraken0509 added 8 commits April 9, 2019 18:59

Add aws_worklink_website_certificate_authority_association Resource

dc25fdb

Add tests for aws_worklink_website_certificate_authority_association …

6e5370f

…resource

Add document for aws_worklink_website_certificate_authority_associati…

05ebe9f

…on resource

Fix resource id format

aace5df

fix error handling

3be5668

Fix usage certificate attribute

504325d

set golangci deadline default(1m) to 5m

4ea8c53

Revert "set golangci deadline default(1m) to 5m"

f9b7c07

This reverts commit 9e34d984560ff4ec526be7636a06124069bc1b5a.

teraken0509 force-pushed the feature/add-support-aws_worklink_website_certificate_authority_association-resource branch from 1db47d3 to f9b7c07 Compare April 9, 2019 10:01

ghost removed the size/XXL Managed by automation to categorize the size of a PR. label Apr 9, 2019

ghost added the size/XL Managed by automation to categorize the size of a PR. label Apr 9, 2019

bflad approved these changes Apr 10, 2019

View reviewed changes

bflad added this to the v2.6.0 milestone Apr 10, 2019

bflad merged commit f5bc5c9 into hashicorp:master Apr 10, 2019

bflad added a commit that referenced this pull request Apr 10, 2019

Update CHANGELOG for #7459

3d70301

teraken0509 deleted the feature/add-support-aws_worklink_website_certificate_authority_association-resource branch March 5, 2020 14:03

ghost locked and limited conversation to collaborators Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support aws worklink website certificate authority association resource #7459

Add support aws worklink website certificate authority association resource #7459

teraken0509 commented Feb 6, 2019 •

edited

Loading

bflad left a comment

bflad Feb 6, 2019

teraken0509 Feb 7, 2019

bflad Feb 6, 2019

teraken0509 Feb 7, 2019

bflad Feb 6, 2019

teraken0509 Feb 7, 2019

bflad Feb 6, 2019

teraken0509 Feb 7, 2019

teraken0509 commented Feb 7, 2019

bflad left a comment

bflad commented Apr 11, 2019

ghost commented Mar 5, 2020

	d.SetId(fmt.Sprintf("%s/%s", aws.StringValue(resp.WebsiteCaId), d.Get("fleet_arn").(string)))
	d.SetId(fmt.Sprintf("%s,%s", d.Get("fleet_arn").(string), aws.StringValue(resp.WebsiteCaId)))


		## Import

		WorkLink Website Certificate Authority can be imported using `WEBSITE-CA-ID/FLEET-ARN`, e.g.

Add support aws worklink website certificate authority association resource #7459

Add support aws worklink website certificate authority association resource #7459

Conversation

teraken0509 commented Feb 6, 2019 • edited Loading

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

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

teraken0509 commented Feb 7, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad commented Apr 11, 2019

ghost commented Mar 5, 2020

teraken0509 commented Feb 6, 2019 •

edited

Loading