-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
GH-1469 - More documentation about using ecs_target. #1470
Conversation
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.
Hey @scream314
Thanks for the work here! 👍
'left a few comments to discuss & address, nothing critical.
* `input` - (Optional) Valid JSON text passed to the target. | ||
* `input_path` - (Optional) The value of the [JSONPath](http://goessner.net/articles/JsonPath/) | ||
that is used for extracting part of the matched event when passing it to the target. | ||
* `role_arn` - (Optional) The Amazon Resource Name (ARN) of the IAM role to be used for this target when the rule is triggered. | ||
* `role_arn` - (Optional) The Amazon Resource Name (ARN) of the IAM role to be used for this target when the rule is triggered. (Required if `ecs_target` is used.) |
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.
Can you remove brackets?
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.
bccb22d
I <3 brackets.
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.
Sorry, loved aussi those brackets, was meant to remove parenthesis, or (
. Dunno bracket was the same for &
(or
)So would go for
Required if ecs_target
is used.`, simply :D
Sorry for the extra work ahah!
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.
No worries.
Cheers.
@@ -68,11 +68,11 @@ The following arguments are supported: | |||
|
|||
* `rule` - (Required) The name of the rule you want to add targets to. | |||
* `target_id` - (Optional) The unique target assignment ID. If missing, will generate a random, unique id. | |||
* `arn` - (Required) The Amazon Resource Name (ARN) associated of the target. | |||
* `arn` - (Required) The Amazon Resource Name (ARN) associated of the target. (It should be the ID of the ECS Cluster if `ecs_target` is used.) |
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.
As you exposed it in your issue, the aws_ecs_cluster.*.id
attribute is the ARN. Since the related documentation exposes it, we should not redo the work here, especially since I'm agrai it will confuse people :/
The other reason is that there are so many edge cases in so many resources, that adding such indications would result in enormous documentation, which we will try to avoid 😄
However, one thing that could be done is the normalization of the data source & resource, so that both uses the arn
too. Does it seem reasonnable to you?
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.
It does.
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.
All good to me, thanks @scream314
Cheers for the extra work 🍻
hashicorpGH-1469 - More documentation about using ecs_target.
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! |
#1469