-
Notifications
You must be signed in to change notification settings - Fork 431
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
Added ECS Task Definition #868
Added ECS Task Definition #868
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Account and 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: ***@***.***>
|
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 @ShubhamPalriwala
@ChristianWitts
Please review the PR
Thanks!