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

Require an HTTP Handler instead of a mux Router in TMDS Config #3666

Merged
merged 2 commits into from
May 1, 2023

Conversation

amogh09
Copy link
Contributor

@amogh09 amogh09 commented Apr 28, 2023

Summary

TMDS initialization function in ecs-agent module currently requires a Router instance from gorilla/mux library to be passed. Usage of Router here is unnecessarily strict and limits the reusability of TMDS to consumers who are using routers from gorilla/mux library. Updating the function to require an http.Handler instance instead which would allow greater reuse. Router implements http.Handler interface.

Implementation details

  • Change router *mux.Router field in TMDS initialization config to handler http.Handler.
  • Update the tests and error messages accordingly.

Testing

New tests cover the changes: yes

Description for the changelog

Require an HTTP Handler instead of a mux Router in TMDS Config to increase the reusability of TMDS.

Licensing

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

@amogh09 amogh09 marked this pull request as ready for review April 28, 2023 23:59
@amogh09 amogh09 requested a review from a team as a code owner April 28, 2023 23:59
@amogh09 amogh09 changed the title Require a HTTP Handler instead of a mux Router in TMDS Config Require an HTTP Handler instead of a mux Router in TMDS Config Apr 29, 2023
@@ -45,7 +45,7 @@ type Config struct {
writeTimeout time.Duration // http server write timeout
steadyStateRate float64 // steady request rate limit
burstRate int // burst request rate limit
router *mux.Router // router with routes configured
handler http.Handler // HTTP handler with routes configured
Copy link
Contributor

@prateekchaudhry prateekchaudhry May 1, 2023

Choose a reason for hiding this comment

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

clarification: is changing from pointer (*mux.Router) to non pointer http.Handler intentional?

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 because http.Handler is an interface whereas mux.Router is a struct.

@amogh09 amogh09 merged commit 81ca75a into aws:dev May 1, 2023
@amogh09 amogh09 deleted the router-to-handler branch May 1, 2023 18:15
@Realmonia Realmonia mentioned this pull request May 9, 2023
Realmonia pushed a commit to Realmonia/amazon-ecs-agent that referenced this pull request May 16, 2023
Realmonia pushed a commit to Realmonia/amazon-ecs-agent that referenced this pull request May 21, 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