Add aws_quicksight_group resource #8233

teraken0509 · 2019-04-06T13:22:22Z

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

Reference #8146

Changes proposed in this pull request:

Add aws_quicksight_group resource

Output from acceptance testing:

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

teraken0509 · 2019-07-17T15:50:48Z

@bflad
Could you review this pr ?

bflad

Hey @kterada0509 👋 Good job as usual. Left some feedback below, please reach out with any questions or if you do not have time to implement the items.

bflad · 2019-07-18T02:58:25Z

aws/resource_aws_quicksight_group.go

+			"namespace": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  "default",


While there are currently no other valid values for this attribute, it also doesn't support in-place updates either so we should include ForceNew: true for now. This can prevent potential issues in the future when other values are supported. 👍

Suggested change

Default: "default",

ForceNew: true,

Default: "default",

bflad · 2019-07-18T03:01:34Z

aws/resource_aws_quicksight_group.go

+
+func resourceAwsQuickSightGroupParseID(id string) (string, string, string, error) {
+	parts := strings.SplitN(id, "/", 3)
+	if parts[0] == "" || parts[1] == "" || parts[2] == "" {


Suggested change

if parts[0] == "" || parts[1] == "" || parts[2] == "" {

if len(parts) < 3 || parts[0] == "" || parts[1] == "" || parts[2] == "" {

bflad · 2019-07-18T03:02:57Z

aws/resource_aws_quicksight_group_test.go

+			return err
+		}
+
+		if output.Group == nil {


Suggested change

if output.Group == nil {

if output == nil || output.Group == nil {

bflad · 2019-07-18T03:05:00Z

aws/resource_aws_quicksight_group_test.go

+			Namespace:    aws.String(namespace),
+			GroupName:    aws.String(groupName),
+		})
+		if !isAWSErr(err, quicksight.ErrCodeResourceNotFoundException, "") {


We generally prefer to split this out so we can display any API errors 👍

if isAWSErr(err, quicksight.ErrCodeResourceNotFoundException, "") { continue } if err != nil { return err } return fmt.Errorf("Quick Sight Group '%s' was not deleted properly", rs.Primary.ID)

bflad · 2019-07-18T03:10:59Z

aws/resource_aws_quicksight_group_test.go

+
+func testAccAWSQuickSightGroupConfig(rName string) string {
+	return fmt.Sprintf(`
+provider "aws" {


For now, please omit hardcoded provider configurations in test configurations and rely on the environment variable handling. We'll eventually be circling around to fix the problems caused by this type of testing setup. 😄 See also: #8316 and #8983

bflad · 2019-07-18T03:17:17Z

aws/resource_aws_quicksight_group.go

+
+			"aws_account_id": {
+				Type:     schema.TypeString,
+				Required: true,


This argument can be made optional by reading the AWS Account ID information from the provider itself, e.g.

awsAccountID := meta.(*AWSClient).accountid namespace := d.Get("namespace").(string) if v, ok := d.GetOk("aws_account_id"); ok { awsAccountID = v.(string) } createOpts := &quicksight.CreateGroupInput{ AwsAccountId: aws.String(awsAccountID), Namespace: aws.String(namespace), GroupName: aws.String(d.Get("group_name").(string)), }

Then the testing can/should omit the aws_caller_identity data source. 👍

Suggested change

Required: true,

Optional: true,

bflad · 2019-07-18T03:18:43Z

website/docs/r/quicksight_group.html.markdown

+  Manages a Resource Quick Sight Group.
+---
+
+# aws_quicksight_group


We likely added this to the other resource pages after this PR was submitted

Suggested change

# aws_quicksight_group

# Resource: aws_quicksight_group

bflad · 2019-07-18T03:19:26Z

website/docs/r/quicksight_group.html.markdown

+
+```hcl
+resource "aws_quicksight_group" "example" {
+	aws_account_id 	= "123456789123"


When this argument is made optional, let's omit this here as well. 👍

bflad · 2019-07-18T03:19:49Z

website/docs/r/quicksight_group.html.markdown

+```hcl
+resource "aws_quicksight_group" "example" {
+	aws_account_id 	= "123456789123"
+	group_name 		= "tf-example"


Nit: formatting

Suggested change

group_name = "tf-example"

group_name = "tf-example"

bflad · 2019-07-18T03:20:21Z

website/docs/r/quicksight_group.html.markdown

+
+* `aws_account_id` - (Required) The ID for the AWS account that the group is in. Currently, you use the ID for the AWS account that contains your Amazon QuickSight account.
+* `group_name` - (Required) A name for the group.
+  given by `source_ami_region`.


This looks extraneous 😉

Suggested change

given by `source_ami_region`.

removed. 😅

teraken0509 · 2019-07-18T11:04:28Z

Re-run acctest

ture/add-support-for-aws_quicksight_group-resource *)$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSQuickSightGroup_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSQuickSightGroup_ -timeout 120m
=== RUN   TestAccAWSQuickSightGroup_basic
=== PAUSE TestAccAWSQuickSightGroup_basic
=== RUN   TestAccAWSQuickSightGroup_withDescription
=== PAUSE TestAccAWSQuickSightGroup_withDescription
=== RUN   TestAccAWSQuickSightGroup_disappears
=== PAUSE TestAccAWSQuickSightGroup_disappears
=== CONT  TestAccAWSQuickSightGroup_basic
=== CONT  TestAccAWSQuickSightGroup_disappears
=== CONT  TestAccAWSQuickSightGroup_withDescription
--- PASS: TestAccAWSQuickSightGroup_disappears (28.91s)
--- PASS: TestAccAWSQuickSightGroup_basic (65.12s)
--- PASS: TestAccAWSQuickSightGroup_withDescription (69.85s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	69.965s

bflad

LGTM, thanks for the updates, @kterada0509 🚀 (One minor note: after activating the subscription it looks like operators choose where the directory is located, so I removed the AWS_DEFAULT_REGION handling in the testing so it can be adjusted by anyone running the testing)

--- PASS: TestAccAWSQuickSightGroup_disappears (10.96s)
--- PASS: TestAccAWSQuickSightGroup_basic (22.27s)
--- PASS: TestAccAWSQuickSightGroup_withDescription (22.77s)

nywilken · 2019-07-19T16:25:32Z

The new aws_quicksight_group resource, has been released in version 2.20.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this resource, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2019-11-02T15:18:59Z

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 force-pushed the feature/add-support-for-aws_quicksight_group-resource branch from 677ed94 to d29ac40 Compare April 9, 2019 09:51

bflad added new-resource Introduces a new resource. service/quicksight Issues and PRs that pertain to the quicksight service. labels Apr 10, 2019

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

bflad suggested changes Jul 18, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 18, 2019

Add aws_quicksight_group resource

ee54c16

teraken0509 force-pushed the feature/add-support-for-aws_quicksight_group-resource branch from d29ac40 to ee54c16 Compare July 18, 2019 11:04

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 18, 2019

bflad approved these changes Jul 18, 2019

View reviewed changes

bflad added this to the v2.20.0 milestone Jul 18, 2019

bflad merged commit ee54c16 into hashicorp:master Jul 18, 2019

bflad added a commit that referenced this pull request Jul 18, 2019

Update CHANGELOG for #8233

c99053e

mjgpy3 mentioned this pull request Oct 6, 2019

Add quicksight user resource #10401

Merged

ghost locked and limited conversation to collaborators Nov 2, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add aws_quicksight_group resource #8233

Add aws_quicksight_group resource #8233

teraken0509 commented Apr 6, 2019

teraken0509 commented Jul 17, 2019

bflad left a comment

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

bflad Jul 18, 2019

teraken0509 Jul 18, 2019

teraken0509 commented Jul 18, 2019

bflad left a comment

nywilken commented Jul 19, 2019

ghost commented Nov 2, 2019

	if parts[0] == "" \|\| parts[1] == "" \|\| parts[2] == "" {
	if len(parts) < 3 \|\| parts[0] == "" \|\| parts[1] == "" \|\| parts[2] == "" {

	if output.Group == nil {
	if output == nil \|\| output.Group == nil {

Add aws_quicksight_group resource #8233

Add aws_quicksight_group resource #8233

Conversation

teraken0509 commented Apr 6, 2019

Community Note

teraken0509 commented Jul 17, 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

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

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 Jul 18, 2019

bflad left a comment

Choose a reason for hiding this comment

nywilken commented Jul 19, 2019

ghost commented Nov 2, 2019