Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add ServiceName to TMDE v4 TaskResponse #3362

Merged
merged 2 commits into from
Sep 8, 2022
Merged

Add ServiceName to TMDE v4 TaskResponse #3362

merged 2 commits into from
Sep 8, 2022

Conversation

amogh09
Copy link
Contributor

@amogh09 amogh09 commented Aug 26, 2022 •
edited
Loading

Summary

Add ServiceName field to TMDE v4 task responses.

Implementation details

ServiceName field is available in Task, so just copying it to TMDE v4 TaskResponse type.

Testing

Tested on an EC2 instance registered under a Gamma cluster. ServiceName field has not yet been added to ACS Task Payload in Production, so the tests have been performed against Gamma environment.

Only v4 Task Responses included ServiceName, v3 responses did not.

[ec2-user@ip-172-31-10-152 amazon-ecs-agent]$ curl http://169.254.170.2/v4/ef23daaf-d681-4cc9-a6df-3f047e25e2fe/task | jq '.ServiceName'                                                                                                                  
"nginx"                                                                                                                      
[ec2-user@ip-172-31-10-152 amazon-ecs-agent]$ curl http://169.254.170.2/v4/ef23daaf-d681-4cc9-a6df-3f047e25e2fe/taskWithTags | jq '.ServiceName'                                                                                                          
"nginx"                                                                        
[ec2-user@ip-172-31-10-152 amazon-ecs-agent]$ curl http://169.254.170.2/v3/ef23daaf-d681-4cc9-a6df-3f047e25e2fe/task | jq '.ServiceName'                                                                                                                  
null                                                                                                                         
[ec2-user@ip-172-31-10-152 amazon-ecs-agent]$ curl http://169.254.170.2/v3/ef23daaf-d681-4cc9-a6df-3f047e25e2fe/taskWithTags | jq '.ServiceName'                                                                                                          
null                                                                                                                         
[ec2-user@ip-172-31-10-152 amazon-ecs-agent]$

New tests cover the changes: Yes

Description for the changelog

Add ServiceName to TMDE v4 Task Responses.

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@amogh09 amogh09 changed the base branch from master to dev August 26, 2022 20:54
@amogh09 amogh09 changed the title WIP: Service tmde Add ServiceName to TMDE v4 TaskResponse Aug 30, 2022
@fierlion
Copy link
Member

can you update the description with a summary of the changes and also talk about how this was tested?

@amogh09 amogh09 changed the title Add ServiceName to TMDE v4 TaskResponse WIP: Add ServiceName to TMDE v4 TaskResponse Aug 30, 2022
@amogh09 amogh09 changed the title WIP: Add ServiceName to TMDE v4 TaskResponse Add ServiceName to TMDE v4 TaskResponse Aug 30, 2022
@amogh09
Copy link
Contributor Author

amogh09 commented Aug 30, 2022

Yeah, updated the description. Should have marked the PR WIP. :)

fierlion
fierlion requested changes Aug 31, 2022
View reviewed changes
agent/handlers/v4/task_metadata_handler.go Outdated
seelog.Infof("V4 taskMetadata handler: Writing response for task '%s'", taskArn)

taskResponse, err := NewTaskResponse(taskArn, state, ecsClient, cluster, az, containerInstanceArn, propagateTags)
taskResponse, err := NewTaskResponse(taskArn, state, ecsClient, cluster,
az, containerInstanceArn, task.ServiceName, propagateTags)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we check that task.ServiceName exists outside the generation of the NewTaskResponse? I see that it's json output is set as omitempty but I still want to be assert that the ServiceName field exists outside this object creation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind -- The zero string value is "" for string type so this will always exist.

ServiceName string              `json:"ServiceName,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we are using empty string to indicate absence of serviceName.

fierlion
fierlion previously approved these changes Aug 31, 2022
View reviewed changes
agent/handlers/v4/task_metadata_handler.go Outdated
seelog.Infof("V4 taskMetadata handler: Writing response for task '%s'", taskArn)

taskResponse, err := NewTaskResponse(taskArn, state, ecsClient, cluster, az, containerInstanceArn, propagateTags)
taskResponse, err := NewTaskResponse(taskArn, state, ecsClient, cluster,
az, containerInstanceArn, task.ServiceName, propagateTags)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind -- The zero string value is "" for string type so this will always exist.

ServiceName string              `json:"ServiceName,omitempty"`

ubhattacharjya
ubhattacharjya previously approved these changes Sep 7, 2022
View reviewed changes
@amogh09 amogh09 dismissed stale reviews from ubhattacharjya and fierlion via a60c2b9 September 8, 2022 01:22
@amogh09 amogh09 merged commit 855df2d into aws:dev Sep 8, 2022
@amogh09 amogh09 deleted the service-tmde branch September 8, 2022 17:08
@chienhanlin chienhanlin mentioned this pull request Sep 12, 2022
Merged
chienhanlin pushed a commit that referenced this pull request Sep 12, 2022
@amogh09 @chienhanlin
Add ServiceName to TMDE v4 TaskResponse (#3362)
1775dcb
@bjrara bjrara mentioned this pull request Jul 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ubhattacharjya ubhattacharjya ubhattacharjya approved these changes

@fierlion fierlion fierlion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amogh09 @fierlion @ubhattacharjya @amazon-ecs-bot