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

Can't override base_url to connect to local PubSub simulator #58

Closed
jeffdeville opened this issue Feb 21, 2018 · 6 comments
Closed

Can't override base_url to connect to local PubSub simulator #58

jeffdeville opened this issue Feb 21, 2018 · 6 comments
Assignees
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release.

Comments

@jeffdeville
Copy link

GoogleApi.PubSub.V1.Connection
uses:
plug Tesla.Middleware.BaseUrl

instead of

Tesla.Middleware.BaseUrlFromConfig

As a result, it's impossible to try and direct commands to the local Pubsub simulator.

@chingor13
Copy link
Contributor

chingor13 commented Apr 11, 2018

Thanks, for the report.

It looks like BaseUrlFromConfig is being removed from Tesla so we'll need to find a different approach to allow configuring the base url.

@jeffdeville
Copy link
Author

jeffdeville commented Apr 11, 2018

**Ok, NM. Just discovered swagger codegen, and it looks like this wasn't your decision to make. **

FWIW, Tesla, like Faraday that it's based on, introduced a ton of complexity for what looked like very little value. I can see where the idea came from, since just about all web frameworks do the same thing, but for an API client, there's really just 3 concerns. Where it's going, how it's secured, and how to format the data you need to send. A pipeline model is simply overkill.

Obviously I shouldn't really care what the underlying tech is here, and perhaps with this swagger code generator you have there's something it adds. But if not, I'd personally consider dropping it in favor of using the HTTPoison.Base macro instead. Short of that, it looks like tesla needs to be flexible enough to take it's various plug parameters as function definitions, and invoke them at runtime to allow things to be configurable.

@chingor13
Copy link
Contributor

So yes, these libraries are all generated via Swagger Codegen. We do maintain our own templates here, so it's possible to swap out Tesla. Originally we picked Tesla because of multipart http posts and the ease of building requests.

We do have a goal to try and create a shared core library which could encapsulate the transport and try and simplify the generated code (and allow better configuration). We could either switch to HTTPoison or provide an equivalent BaseUrlFromConfig implementation.

Alternatively, we can look to simply swap out the middleware if we're ok locking the 0.x (the middleware is being removed in 1.0), but it could conflict in the future.

@dazuma dazuma added the priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. label May 11, 2018
@teamon
Copy link

teamon commented May 23, 2018

Disclaimer: I'm the author of tesla

BaseUrlFromConfig middleware was removed because it is not needed, you can achieve the same thing using:

plug Tesla.Middleware.BaseUrl, fetch_base_url_however_you_like()
# or
plug Tesla.Middleware.BaseUrl, Application.get_env(:google, :base_uri)

Middleware arguments are evaluated at runtime so everything will work just fine.

@florinpatrascu
Copy link

@teamon - I did that in my fork of the mustache templates, and it works very well, even closer to the initial intent of the author?! That of using a package specific env var. In my fork:

    Tesla.build_client([
      {Tesla.Middleware.BaseUrl, env_base_url()},
      {Tesla.Middleware.Headers, [{"Authorization", "Bearer #{token}"}]}
    ])

where the env_base_url is as simple as this:

  defp env_base_url() do
    Application.get_env(:{{#underscored}}{{packageName}}{{/underscored}}, :url, "{{{basePath}}}")
  end

@chingor13 - please observe the use of packageName rather than the appName, as the latter will result in something like: :google_cloud_pub/sub_api, an invalid Elixir Atom, of course.

@chingor13
Copy link
Contributor

We just released new versions of the client libraries which includes the fix from #81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release.
Projects
None yet
Development

No branches or pull requests

5 participants