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

Move ACS client to ecs-agent module and refactor #3710

Merged
merged 1 commit into from
May 25, 2023

Conversation

danehlim
Copy link
Contributor

@danehlim danehlim commented May 23, 2023

Summary

Use ecs-agent module's wsclient in ACS, move ACS client to ecs-agent module, and refactor associated code.

Implementation details

  1. In agent/acs/, swap out the following imports in all files:
    • github.com/aws/amazon-ecs-agent/agent/wsclient with github.com/aws/amazon-ecs-agent/ecs-agent/wsclient
    • github.com/aws/amazon-ecs-agent/agent/wsclient/mock with github.com/aws/amazon-ecs-agent/ecs-agent/wsclient/mock
    • github.com/aws/amazon-ecs-agent/agent/wsclient/wsconn with github.com/aws/amazon-ecs-agent/ecs-agent/wsclient/wsconn
  2. Refactor agent/acs/client and move to ecs-agent/
    • agent/acs/client/acs_client.go -> ecs-agent/acs/client/acs_client.go
      • Update functions such that they conform to wsclient defined in ecs-agent module
      • Use github.com/aws/amazon-ecs-agent/ecs-agent/logger instead of github.com/cihub/seelog for logging
      • Define new empty acsClientFactory struct and refactor New() function into acsClientFactory struct method, such that acsClientFactory struct implements wsclient.ClientFactory interface
        • This wsclient.ClientFactory interface returns a new wsclient.ClientServer and helps in testing session/ACS handler code, which is critical for ACS (we can now easily test and define what to expect during session's creation of ACS client/how the ACS client came to be in the first place)
        • The size of the acsClientFactory struct itself is zero (since it contains no fields) and thus does not introduce any need for additional storage
    • agent/acs/client/acs_client_test.go -> ecs-agent/acs/client/acs_client_test.go
      • Update tests accordingly based off of changes to ACS client above
    • agent/acs/client/acs_client_types.go -> ecs-agent/acs/client/acs_client_types.go
      • Add ACS confirm attachment message to ACS recognized types
    • ecs-agent/acs/client/acs_error.go -> ecs-agent/acs/client/acs_error.go
    • ecs-agent/acs/client/acs_error_test.go -> ecs-agent/acs/client/acs_error_test.go
  3. Refactor agent/acs/handler/acs_handler.go and associated files
    • agent/acs/handler/acs_handler.go
      • Account for previous implementation detail's changes to ACS client
      • Factor out over-engineered interfaces, structs, and methods related to sessionResources. Instead, simply use client factory for creating ACS client and testing the same. This results in less code that is simpler and more maintainable
    • agent/acs/handler/acs_handler_test.go
      • Update tests accordingly based off of changes to agent/acs/handler/acs_handler.go
    • agent/app/agent.go
      • Provide ACS client factory in call to acshandler.NewSession function based off of changes to agent/acs/handler/acs_handler.go

Testing

Unit, integration, and functional tests.

New tests cover the changes: yes

Description for the changelog

Move ACS client to ecs-agent module and refactor

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 May 23, 2023 23:08
@danehlim danehlim requested a review from a team as a code owner May 23, 2023 23:08
ubhattacharjya
ubhattacharjya previously approved these changes May 24, 2023
@danehlim danehlim merged commit d534247 into aws:dev May 25, 2023
@yinyic yinyic mentioned this pull request Jun 6, 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.

4 participants