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.

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

Consume ECS client from ecs-agent module in agent module #4032

Merged
merged 1 commit into from Nov 20, 2023

Conversation

danehlim
Copy link
Contributor

@danehlim danehlim commented Nov 16, 2023

Summary

In the agent module, consume the ECS client defined in ecs-agent module.

Implementation details

  • Replace all ECS client and corresponding mocks usages with the ECS client and corresponding mocks defined in ecs-agent module and update tests accordingly where necessary
  • Make current NewContainerMetadataGetter and NewTaskMetadataGetter functions defined in agent/api/metadata_getter.go private as they are not to be used outside of the agent module's api package
  • Write ToECSAgent translation methods for agent module level TaskStateChange, ContainerStateChange, and AttachmentStateChange state change types that convert to the state change types defined in the ecs-agent module
    • NOTE: Struct ProtocolBindIP and helper functions buildManagedAgentStateChangePayload and buildContainerStateChangePayload introduced as part of this pull request in agent/api/statechange.go are based off the components with the same names currently defined in agent/api/ecsclient/client.go
  • Define new utils function Int64PtrToIntPtr in ecs-agent/utils/utils.go for use in ToECSAgent translation method
  • In ecs-agent/acs/session/session.go, refactor session struct field discoverEndpointClient of type api.ECSDiscoverEndpointSDK such that it is renamed to struct field ecsClient with type ecs.ECSClient instead
    • Type api.ECSDiscoverEndpointSDK is currently being used in ACS session but is no longer necessary now that there exists an ECS client in the ecs-agent module that we are consuming in agent module as part of this pull request
  • Add a unit test in agent/api/task/task_test.go to assert that container port zero will explicitly not be included in the set of container ports that are exposed for a container
  • Delete the following files as they are no longer being used:
    • agent/api/ecsclient/client.go
    • agent/api/ecsclient/client_test.go
    • agent/api/ecsclient/retry_handler.go
    • agent/api/ecsclient/retry_handler_test.go
    • agent/api/ecsclient/utils.go
    • agent/api/ecsclient/utils_amd64_test.go
    • agent/api/ecsclient/utils_arm64_test.go
    • agent/api/generate_mocks.go
    • agent/api/interface.go
    • agent/api/mocks/api_mocks.go
    • ecs-agent/api/generate_mocks.go
    • ecs-agent/api/interface.go
    • ecs-agent/api/mocks/api_mocks.go

Testing

Unit, integration, and functional tests.

New tests cover the changes: yes

Description for the changelog

Consume ECS client from ecs-agent module in agent module

Does this PR include breaking model changes? If so, Have you added transformation functions?
No

Licensing

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

@danehlim danehlim marked this pull request as ready for review November 16, 2023 01:51
@danehlim danehlim requested a review from a team as a code owner November 16, 2023 01:51
@danehlim danehlim force-pushed the ecsclient-agent-integration branch 3 times, most recently from e5b9177 to 44eb7c2 Compare November 17, 2023 21:42
fierlion
fierlion previously approved these changes Nov 20, 2023
mye956
mye956 previously approved these changes Nov 20, 2023
Copy link
Member

@fierlion fierlion left a comment

Choose a reason for hiding this comment

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

approve (post merge)

@danehlim danehlim merged commit 5f35c93 into aws:dev Nov 20, 2023
38 checks passed
@danehlim danehlim mentioned this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants