Added ECS Task Definition #868
Conversation
There was a problem hiding this comment.
Thanks @aimanafzal for the PR
You need to invoke the function [here] :)(https://github.com/tailwarden/komiser/blob/develop/providers/aws/aws.go#L38)
My bad. |
Thanks, can you fix the conflicts as well? :) |
|
Please take a look at it. |
There was a problem hiding this comment.
Hey @aimanafzal, thanks a lot for this PR! I just tested it locally and I can see the resource 🚀 :
However, I would be really glad if you could possibly make the following fixes as well and then we should be gtg:
- Fix the name to show the actual ECS Task Definition name (store-demo-containers in my case)
- Fix the URL because it currently does not redirect me to the task definition and rather takes me to https://eu-north-1.console.aws.amazon.com/ecs/v2/getStarted?region=eu-north-1
|
Cool. |
|
Hey @aimanafzal let me know if you are stuck somewhere or need any help here 🙏🏼 |
|
Hi Shubham, sorry for responding a bit late.
I could not find a way to display the actual ECS Task Definition Name. Every file is having the service name instead of the actual name.
Let me know if there is a way to do it.
Or we can jump on a call later this week if that is possible :)
On 04-Jul-2023, at 7:31 PM, Shubham Palriwala ***@***.***> wrote:
Hey @aimanafzal<https://github.com/aimanafzal> let me know if you are stuck somewhere or need any help here 🙏🏼
—
Reply to this email directly, view it on GitHub<#868 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACDSA753BFAQNU4ENZG67CLXOQSKZANCNFSM6AAAAAAZVHY6JU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Hey @aimanafzal, you can parse the ECS Task def name and the revision w the below function: func extractNameAndRevisionFromArn(input string) []string {
var nameAndRevision [2]string
parts := strings.Split(input, ":")
if len(parts) >= 6 {
nameAndRevision[0] = strings.Split(parts[5], "/")[1]
nameAndRevision[1] = parts[6]
}
return nameAndRevision[:]
}And call it like this: ecsNameAndRevision := extractNameAndRevisionFromArn(taskdefinition)Now you can append the resource as: resources = append(resources, Resource{
Provider: "AWS",
Account: client.Name,
Service: "ECS Task Definition",
ResourceId: taskdefinition,
Region: client.AWSClient.Region,
Name: ecsNameAndRevision[0],
Cost: 0,
FetchedAt: time.Now(),
Link: fmt.Sprintf("https://%s.console.aws.amazon.com/ecs/v2/task-definitions/%s/%s/containers?region=%s", client.AWSClient.Region, ecsNameAndRevision[0], ecsNameAndRevision[1], client.AWSClient.Region),
})Another small overhead I found is the calculation of the resource Arn which we do not need hence you can remove the below code from your file: stsClient := sts.NewFromConfig(*client.AWSClient)
stsoutput, err := stsClient.GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{})
if err != nil {
return resources, err
}
accountId := stsoutput.Accountand the ResourceId can directly become the This should solve and get the PR merged, as I have tested it locally as well. Let me know what you think 💯 |
|
Thanks for sharing.
Let me work on it 👍
On 06-Jul-2023, at 1:56 PM, Shubham Palriwala ***@***.***> wrote:
Hey @aimanafzal<https://github.com/aimanafzal>, you can parse the ECS Task def name and the revision w the below function:
func extractNameAndRevisionFromArn(input string) []string {
var nameAndRevision [2]string
parts := strings.Split(input, ":")
if len(parts) >= 6 {
nameAndRevision[0] = strings.Split(parts[5], "/")[1]
nameAndRevision[1] = parts[6]
}
return nameAndRevision[:]
}
And call it like this:
ecsNameAndRevision := extractNameAndRevisionFromArn(taskdefinition)
Now you can append the resource as:
resources = append(resources, Resource{
Provider: "AWS",
Account: client.Name,
Service: "ECS Task Definition",
ResourceId: taskdefinition,
Region: client.AWSClient.Region,
Name: ecsNameAndRevision[0],
Cost: 0,
FetchedAt: time.Now(),
Link: fmt.Sprintf("https://%s.console.aws.amazon.com/ecs/v2/task-definitions/%s/%s/containers?region=%s", client.AWSClient.Region, ecsNameAndRevision[0], ecsNameAndRevision[1], client.AWSClient.Region),
})
Another small overhead I found is the calculation of the resource Arn which we do not need hence you can remove the below code from your file:
stsClient := sts.NewFromConfig(*client.AWSClient)
stsoutput, err := stsClient.GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{})
if err != nil {
return resources, err
}
accountId := stsoutput.Account
and the ResourceId can directly become the taskdefinition!
This should solve and get the PR merged, as I have tested it locally as well. Let me know what you think 💯
—
Reply to this email directly, view it on GitHub<#868 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACDSA74CWXCJKPHZIFKTR7TXOZ4S5ANCNFSM6AAAAAAZVHY6JU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Hey @aimanafzal, I made a small correction in the URL for the resource and the PR is good to be merged now! thanks a lot for building this feature 💯 @mlabouardy please take a look at the changes once and then i'll proceed! |
|
Perfect!
I wanted to make the same comment about checking the URL.
Thanks Mate.
Really appreciated.
On 10-Jul-2023, at 6:42 PM, Shubham Palriwala ***@***.***> wrote:
Hey @aimanafzal<https://github.com/aimanafzal>, I made a small correction in the URL for the resource and the PR is good to be merged now! thanks a lot for building this feature 💯
@mlabouardy<https://github.com/mlabouardy> please take a look at the changes once and then i'll proceed!
—
Reply to this email directly, view it on GitHub<#868 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACDSA7ZDKI76PZEGXN4GK3TXPQBFFANCNFSM6AAAAAAZVHY6JU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@ChristianWitts
Please review the PR
Thanks!