Closed vrcca closed 3 years ago
Thank you @vrcca!
I haven't played with the generator yet but I am thinking it may be better to keep the bulk of this code on AWS Elixir. In particular, AWS Elixir can ship with two modules: AWS.JSON
and AWS.HTTP
. Those modules will have the entry point functions AWS.JSON.encode
, AWS.JSON.decode
and AWS.HTTP.request
and in this project we change the generator to use these new modules but without any specific semantics of the application environment. In fact, the default implementation of these functions can continue using Poison+HTTPoison. Then, when we want to use the app environment, we only need to change aws-elixir and keep the coupling here minimal. What do you think?
Do we already have any other files in aws-elixir that are part of the project instead of being auto-generated?
Do we already have any other files in aws-elixir that are part of the project instead of being auto-generated?
AWS.Request seems to be such an example. :)
I haven't played with the generator yet but I am thinking it may be better to keep the bulk of this code on AWS Elixir. In particular, AWS Elixir can ship with two modules:
AWS.JSON
andAWS.HTTP
. Those modules will have the entry point functionsAWS.JSON.encode
,AWS.JSON.decode
andAWS.HTTP.request
and in this project we change the generator to use these new modules but without any specific semantics of the application environment. In fact, the default implementation of these functions can continue using Poison+HTTPoison. Then, when we want to use the app environment, we only need to change aws-elixir and keep the coupling here minimal. What do you think?
I like this! Tonight I will have some spare time and will update the code to reflect this 😄
Hi @vrcca, these changes look great! I have discussed this with my team today and we have one last suggestion/request. What do you think about passing the client both to AWS.HTTPClient.request and AWS.JSON.decode? The rationale is that we should not be using Application.get_env
but instead we should configure the HTTPClient
and the JSON
decoder in the AWS client struct. This means it is easier to test, easy to target different clients, and so on. Thoughts?
Hi @vrcca, these changes look great! I have discussed this with my team today and we have one last suggestion/request. What do you think about passing the client both to AWS.HTTPClient.request and AWS.JSON.decode? The rationale is that we should not be using
Application.get_env
but instead we should configure theHTTPClient
and theJSON
decoder in the AWS client struct. This means it is easier to test, easy to target different clients, and so on. Thoughts?
Hmm.. I think it makes more sense since we already build a client before calling the API.
So just to confirm that I understand it correctly. Your proposal is something like this?
Calling aws-elixir:
iex> client = %AWS.Client{access_key_id: "<access-key-id>",
secret_access_key: "<secret-access-key>",
region: "us-east-1",
endpoint: "amazonaws.com"
http_client: AWS.HTTP, # optional, must implement request/5. Defaults to AWS.HTTP
json_handler: Jason, # optional, must implement decode!/1 & encode!/1. Defaults to AWS.JSON
xml_handler: AWS.XML} # optional, must implement decode!/1 & encode!/1. Defaults to AWS.XML
iex> {:ok, result, resp} = AWS.Kinesis.list_streams(client, %{})
The generator:
case Map.fetch!(client, :http_client).request(:post, url, payload, headers, options) do
# `context.content_type` prints either :json or :xml
body = if(body != "", do: Map.fetch!(client, <%= context.content_type %>_handler).decode!(body))
{:ok, body, response}
Wdyt? It would be great to know what you think @jfacorro!
Exactly!
I would even go as far as making the http_client/json be: {Mod, options}
where options are passed as an extra argument. This can be useful for some HTTP clients that requires the pool name or similar. But that's a separate discussion, for now we just need to pass the client. :)
@josevalim This is how it looks now AWS.Client. I couldn't think of better names or structure, so feel free to send more suggestions! 😄
@vrcca I think we could restrict this PR only to the Elixir code generator and let the work for the Erlang one to another PR. WDYT?
@philss Honestly, I don't know much Erlang. I was trying to do the same changes there, but I was struggling a bit. I like the idea though. I will change the PR description.
So is this ready to go? :)--
José Valimhttps://dashbit.co/ https://dashbit.co/
Thanks @philss & @josevalim for helping me! It was really great & I learned a lot! 😄
@vrcca thank you for ~the~ your contribution! :purple_heart:
This PR allows the users of the aws-elixir generated code to change how it handles HTTP calls and JSON encoding/decoding by delegating the work to
AWS.Client
implementations.The result of this PR can be found here:
Example:
TODO