-
Notifications
You must be signed in to change notification settings - Fork 619
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
Refactor introspection api to consume shared implementation #4483
Conversation
This PR was split off from Move introspection server to ecs-agent into two pull requests:
|
ad3329f
to
7ad2564
Compare
7ad2564
to
d63fb91
Compare
d63fb91
to
a9b2faa
Compare
e603fef
to
df0d75b
Compare
df0d75b
to
1881965
Compare
1881965
to
e6dfb38
Compare
f101158
to
dcad0f7
Compare
if !found { | ||
return nil, v1.NewErrorNotFound(fmt.Sprintf("no task found with %s %s", keyType, key)) | ||
} | ||
containerMap, _ := agentState.ContainerMapByArn(task.Arn) |
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.
We are doing a lookup here, we should handle "not found" error here too imho
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.
I see that this is similar to current implementation, and in case ContainerMapByArn
happens to return nil, the containers list will be empty
Summary
As part of the effort to bring the Agent Introspection Server to the Fargate Agent, this change refactors the agent
introspection
server to consume the shared implementation inecs-agent
.Implementation details
The http server logic for the introspection server has been moved to the shared
ecs-agent
library. The server depends on theAgentState
interface which will be implemented in the ECS and Fargate agents. TheAgentState
implementations will supply the server with the data it needs to construct responses. This change refactors the ECS agent's introspection server to consume the shared introspection server.Testing
Unit and integration tests
Existing unit and integration tests have been modified, and where applicable, additional tests have been added. Existing
unit
tests forintrospection_server
inagent/handlers/v1
were moved to anintegration
test file with the intent to:Manual tests
Outputs from the Agent introspection server were recorded from two test clusters: one using the current version of the ECS agent, and the other using a modified build with the shared introspection server implementation. E.g.,
/v1/metadata
Current implementation
Shared implementation
New tests cover the changes: yes
Description for the changelog
Enhancement - Refactor introspection api to consume shared server in ecs-agent
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
No
Does this PR include the addition of new environment variables in the README?
No
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.