New Resource: aws_media_store_container #2448

atsushi-ishibashi · 2017-11-27T17:04:01Z

Required: #2447

make testacc TEST=./aws TESTARGS='-run=TestAccAwsMediaStoreContainer_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsMediaStoreContainer_basic -timeout 120m
=== RUN   TestAccAwsMediaStoreContainer_basic
--- PASS: TestAccAwsMediaStoreContainer_basic (135.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	135.666s

radeksimko

Tests are passing, thanks for the PR - I left you some comments in the code.

radeksimko · 2017-12-02T04:21:25Z

aws/resource_aws_media_store_container.go

+
+	_, err = stateConf.WaitForState()
+	if err != nil {
+		return fmt.Errorf("[WARN] Error waiting for MediaStore Container status to be \"ACTIVE\": %s", err)


Nitpick, but I think the error which comes out of WaitForState is sufficiently detailed so we don't need to duplicate the context here.

radeksimko · 2017-12-02T04:22:58Z

aws/resource_aws_media_store_container.go

+
+	if !d.HasChange("policy") {
+		return resourceAwsMediaStoreContainerRead(d, meta)
+	}


I believe the above 3 lines will never be executed since all fields are ForceNew (so the only way the function ever gets executed is when policy has changed), unless I'm mistaken?

I called resourceAwsMediaStoreContainerUpdate from the end of resourceAwsMediaStoreContainerCreate so I needed the above 3 lines.
Separating policy as aws_media_store_container_policy will solve this complication.

Ah, I see - that's confusing. It actually makes me reconsider what I said in my other comment - I think we should remove all the policy related logic from this PR and implement it as a separate resource in another PR.

Do you agree?

Yes, I agree👍 I'll remove

radeksimko · 2017-12-02T04:24:30Z

aws/resource_aws_media_store_container.go

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


Nitpick, but I think we could simplify the above error handling to something like

if isAWSErr(err, mediastore.ErrCodeContainerNotFoundException, "") { d.SetId("") return nil }

radeksimko · 2017-12-02T04:25:20Z

aws/resource_aws_media_store_container_test.go

+				case mediastore.ErrCodeContainerNotFoundException:
+					return nil
+				}
+			}


As mentioned elsewhere - I think the error handling can be simplified a bit.

radeksimko · 2017-12-02T04:26:11Z

aws/resource_aws_media_store_container.go

+			}
+		}
+		return err
+	}


As this resource is stateful shouldn't we also wait here until it's deleted instead of leaving it in "deleting" state?

radeksimko · 2017-12-02T04:28:25Z

aws/resource_aws_media_store_container_test.go

+    "Action": [ "mediastore:*" ],
+    "Principal": {"AWS" : "*"},
+    "Effect": "Allow",
+    "Resource": "arn:aws:mediastore:us-west-2:${data.aws_caller_identity.test.account_id}:container/tf_mediastore_%s/*",


It looks like we'd benefit from having aws_media_store_container_policy so we can reference the container ARN instead of building it like this. 🤔

It's certainly outside of scope of this PR, just thinking out loud as I'm reading the code...

Yeah, I agree with you...
"Resource" in policy must be scope of arn:aws:mediastore:us-west-2:1234567890:container/tf_mediastore_%s/ (I got error if I set * to Resource) so I thought the same when implementing.
If we'll develop aws_media_store_container_policy, should I remove policy from aws_media_store_container to simplify relation and these roles?

No, keep it here... as I said it's outside of scope of this PR 😃

I'm taking back what I said - see my other comment about policy.

radeksimko · 2017-12-02T04:31:04Z

website/docs/r/media_store_container.html.markdown

+
+The following arguments are supported:
+
+* `name` - (Required) The name of the container. Must atisfy regular expression pattern: `\w+`


Can we make this a little bit more human-readable for folks who don't know regular expressions or omit this sentence (as the validation error message will tell them anyways)?

radeksimko · 2017-12-02T04:32:47Z

aws/resource_aws_media_store_container.go

+				ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
+					value := v.(string)
+					if !regexp.MustCompile("^\\w+$").MatchString(value) {
+						errors = append(errors, fmt.Errorf("%q must match \\w+", k))


Can we make this error message a bit more human friendly for folks who don't know regular expressions? e.g. must contain alphanumeric characters or underscores?

atsushi-ishibashi · 2017-12-07T08:56:09Z

I removed policy. It's ready to review👍

radeksimko

LGTM, thanks.

* Add Data Source: aws_elb * Fix dataSourceAwsElb typo * Fix dataSourceAwsElb Schema name field to not include Computed * Remove dataSourceAwsElb schema defaults for computed fields * Remove dataSourceAwsElb schema defaults for nested computed fields too * Corrected depends_on entry for EIP depends_on = ["aws_internet_gateway.gw"] is the correct syntax * Add sweeper for IAM Server Certificates * test/aws_config_delivery_channel: Add missing dependencies * d/aws_elb r/aws_elb: hashicorp#2004 review comments * Remove enable_deletion_protection from testAccDataSourceAWSELBConfigBasic * Replace unnecessary errwrap.Wrapf with fmt.Errorf * Reduce flattenAwsELbResource to ec2conn and elbconn instead of meta * Properly name TestAccDataSourceAWSELB_basic resources * Use t.Name() for description and TestName tags * d/aws_elb: Fix documentation sidebar ordering after merging master with new d/aws_elasticache_replication_group * Makefile: Add sweep target * Update cognito_user_pool.markdown * Update CHANGELOG.md * r/aws_elasticache_security_group: add import support (hashicorp#2277) * r/aws_elasticache_security_group: add import support * r/aws_elasticache_security_group: hashicorp#2277 review updates * Use d.Id() instead of d.Get("name") on read, which allows using schema.ImportStatePassthrough * d.Set("security_group_names") on read * Set AWS_DEFAULT_REGION to us-east-1 on import testing * Update CHANGELOG.md * documentation: remove antislashes in page titles * Added missing WARN debug lines when reading a non-existing resource * Removed <wbr> from documentation titles * vendor: Bump aws-sdk-go to v.1.12.44 * Add logs for iam server certificate delete conflict (hashicorp#2533) * Query elb API for load balancer arn causing delete conflict - For IAM server certificate. * Use regex for lb name. * Edits for hashicorp#2533 * r/aws_sqs_queue_policy: Support import by queue URL (hashicorp#2544) * Update CHANGELOG.md * r/aws_elasticsearch_domain: Add LogPublishingOption (hashicorp#2285) * WIP * Add enabled * Use cwl policy * Reflect reviews * Update CHANGELOG.md * Add force_destroy field to aws_athena_database (hashicorp#2363) * Add force_destroy field to aws_athena_database. Fixes hashicorp#2362. * Remove unnecessary import. * Code review feedback * Update CHANGELOG.md * Add more example and missing field * New Resource: aws_media_store_container (hashicorp#2448) * New Resource: aws_media_store_container * Reflect reviews * remove policy * Update CHANGELOG.md * Add Redis AUTH, in-transit and at-rest encryption (hashicorp#2090) * add AUTH, at-rest and in-transit encryption to Elasticache replication groups * add _enabled to transit/at_rest encyrption parameters * added one more _enabled * move validateAwsElastiCacheReplicationGroupAuthToken to aws/validators.go, as well as tests * set auth_token to nil during Reads * update Replication Group encryption acceptance tests to use config functions instead of vars * Fix whitespacing (tabs -> spaces) * docs/elasticache_replication_group: Add missing fields * Update CHANGELOG.md * r/aws_dynamodb_table: Ensure ttl is properly read (hashicorp#2452) * r/aws_dynamodb_table: Ensure ttl is properly read * r/aws_dynamodb_table: hashicorp#2452 review updates * Add timeToLiveOutput.TimeToLiveDescription nil check * Simplify logic to d.Set ttl * Update CHANGELOG.md * Bump aws-sdk-go to v.1.12.45 * New Resource: PublicDnsNamespace (hashicorp#2569) * WIP * Add test, docs * Reflect reviews * Modify error handling * Update CHANGELOG.md * New Resource: ServiceDiscovery PrivateDNS Namespace (hashicorp#2589) * New Resource: service_discovery_private_dns_namespace * Reflect reviews * Update CHANGELOG.md

ghost · 2020-04-10T17:18:22Z

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!

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 29, 2017

New Resource: aws_media_store_container

f248e51

atsushi-ishibashi force-pushed the aws_media_store_container branch from ce0d3fa to f248e51 Compare December 1, 2017 12:55

radeksimko reviewed Dec 2, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 2, 2017

atsushi-ishibashi added 2 commits December 3, 2017 11:59

Reflect reviews

50b5c7b

remove policy

58c7ac7

radeksimko approved these changes Dec 11, 2017

View reviewed changes

radeksimko merged commit 97942bf into hashicorp:master Dec 11, 2017

atsushi-ishibashi deleted the aws_media_store_container branch December 13, 2017 15:16

atsushi-ishibashi mentioned this pull request Dec 20, 2017

Support MediaStore #2446

Closed

ghost locked and limited conversation to collaborators Apr 10, 2020

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_media_store_container #2448

New Resource: aws_media_store_container #2448

atsushi-ishibashi commented Nov 27, 2017

radeksimko left a comment

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

atsushi-ishibashi Dec 3, 2017

radeksimko Dec 4, 2017

atsushi-ishibashi Dec 4, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

atsushi-ishibashi Dec 3, 2017 •

edited

Loading

radeksimko Dec 4, 2017

radeksimko Dec 4, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

atsushi-ishibashi commented Dec 7, 2017

radeksimko left a comment

ghost commented Apr 10, 2020


		The following arguments are supported:

		* `name` - (Required) The name of the container. Must atisfy regular expression pattern: `\w+`

New Resource: aws_media_store_container #2448

New Resource: aws_media_store_container #2448

Conversation

atsushi-ishibashi commented Nov 27, 2017

radeksimko 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

atsushi-ishibashi Dec 3, 2017 • edited Loading

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

atsushi-ishibashi commented Dec 7, 2017

radeksimko left a comment

Choose a reason for hiding this comment

ghost commented Apr 10, 2020

atsushi-ishibashi Dec 3, 2017 •

edited

Loading