fix(cli): best-effort create service linked role for ECS on env init#1749
fix(cli): best-effort create service linked role for ECS on env init#1749mergify[bot] merged 3 commits intoaws:mainlinefrom
Conversation
bvtujo
left a comment
bvtujo
left a comment
There was a problem hiding this comment.
Looks good, just a couple of questions
| // 1. Attempt to create the service linked role if it doesn't exist. | ||
| // If the call fails because the role already exists, nothing to do. | ||
| // If the call fails because the user doesn't have permissions, then the role must be created outside of Copilot. | ||
| _ = o.iam.CreateECSServiceLinkedRole() |
There was a problem hiding this comment.
Are those the only two failure cases? Is it okay not to fail here due to permissions issues without surfacing any error?
There was a problem hiding this comment.
I feel like it's fine because I don't' know what more we can do here for the user:
If the user doesn't have permission and the role was already created -> we don't want to fail.
If there was an internal error -> we don't want to fail.
For all other situations -> same behavior as today, it will print that the role is missing.
| if _, err := c.client.CreateServiceLinkedRole(&iam.CreateServiceLinkedRoleInput{ | ||
| AWSServiceName: aws.String(ecsServiceName), | ||
| }); err != nil { | ||
| return fmt.Errorf("create service linked role for Amazon ECS: %w", err) |
There was a problem hiding this comment.
Do we need to wrap this error, since it a) doesn't ever get reported and b) will always be wrapped by other methods?
There was a problem hiding this comment.
Hmm good question 🤔, I think:
a) That should be up to the client to decide whether they want to ignore the error or handle it, so we should still return the error.
b) It's definitely odd reading: "create ECS service linked role: create ECS service linked role: AWS err"
I think we should still wrap but I'll change the data of this error message so that it's more specific to the input of the call:
"create service linked role for ecs.amazonaws.com: %w"
…ws#1749) For accounts that never used ECS before, the [service linked role](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/using-service-linked-roles.html) does not exist which results in ECS not being able to create the cluster during `env init`. This change attempts a best-effort creation of the role before creating the environment stack. Resolves aws#1324 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
4 participants
For accounts that never used ECS before, the service linked role does not exist which results in ECS not being able to create the cluster during
env init.This change attempts a best-effort creation of the role before creating the environment stack.
Resolves #1324
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.