-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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.
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
provider/aws: Allow ARN identifier to be set #11359
Conversation
Allows users from govcloud and other regions (aws-cn) to now use the following resources correctly: ``` - data "aws_billing_service_account" - data "aws_elb_service_account" - resource "aws_cloudfront_origin_access_identity" - resource "aws_ecs_service" - resource "aws_iam_saml_provider" - resource "aws_lambda_permission" - resource "aws_sns_topic_policy" ```
@@ -292,7 +292,7 @@ func resourceAwsEcsServiceRead(d *schema.ResourceData, meta interface{}) error { | |||
d.Set("desired_count", service.DesiredCount) | |||
|
|||
// Save cluster in the same format | |||
if strings.HasPrefix(d.Get("cluster").(string), "arn:aws:ecs:") { | |||
if regexp.MustCompile(`^arn:[\w-]+:ecs:`).MatchString(d.Get("cluster").(string)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I originally suggested using regexp matching here, but now that I'm looking at the whole diff I'm thinking we could just use the same pattern as elsewhere and avoid the loose matching - i.e.
if strings.HasPrefix(d.Get("cluster").(string), "arn:"+meta.(*AWSClient).partition+":ecs:") {
or something similar w/ fmt.Sprintf
- depending on what looks better.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks/sounds good to me, I'll make that change and push
if qualifier != expectedQualifier { | ||
t.Fatalf("Expected qualifier to match (%q != %q)", qualifier, expectedQualifier) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overall looks good, although it would be great if we could unify the way we do the pattern matching as mentioned.
It's not a blocker though. 👍
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Allows users from govcloud and other regions (aws-cn) to now use the following resources correctly:
Fixes: #7309
Related: