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

Allow to specify HTTPoison options directly in config #38

Closed
wants to merge 1 commit into from

Conversation

oltarasenko
Copy link
Collaborator

Made in order to address the: #33

using custom options like: follow_redirect or proxy.
@Ziinc
Copy link
Collaborator

Ziinc commented Dec 20, 2019

From my understanding of worker.ex, the HTTPoison options are already getting passed to the HTTPoison.get through the :options key on the Crawly.Request struct

In worker.ex:69:
image

I don't think additional global configs are required. Just need additional documentation on how to create a custom middlewares to add in request options

@Ziinc
Copy link
Collaborator

Ziinc commented Dec 20, 2019

Additionally, it would be more appropriate to abstract out the :proxy and :follow_redirects global options to individual middlewares.

Also, I notice that the follow_redirects config key is being used or passed to HTTPoison at all.

I'm going to be finishing up #31 today, could you use that as a basis for writing the middlewares for the two options (proxy and follow_redirects)? I will be adding in docs on #31 on how to write a custom middleware, so it won't be necessary to do so on this branch
Maybe add in docs for adding HTTPoison options through a middleware?

Another note is that the proxy middleware needs to accept two options, the proxy host and port, and a proxy_auth key as well, as HTTPoison doesn't parse a proxy string in the format of http://user:pass@host:port.
I'll chip in once I finish #31

Copy link
Collaborator

@Ziinc Ziinc left a comment

Choose a reason for hiding this comment

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

As mentioned in my comment, I think we should be reducing the use of global configs.

For the case of Crawly.fetch, I think what would make it better would be to instead to call relevant functions in RequestsStorageWorker and DataStorageWorker to simulate the request moving through the declared middlewares & pipelines

Also, there seems to be a typo for the :follow_redirect key, it should be without an s.

@oltarasenko
Copy link
Collaborator Author

Hmm... Yeah @Ziinc , it's complicated I agree.

I kind of agree with your comments!
I think I can try using the middlewares for this purpose as well, however, I am not sure we should do
it. E.g. I prefer to use middlewares for things which require more-or-less complicated ongoing operations on the requests.

So to say from my point of view it probably makes sense using middlewares for the case of proxies (where we might want to direct requests via different sources). But maybe I would not want to use them to hardcode the ssl options.

What do you think?

@Ziinc
Copy link
Collaborator

Ziinc commented Dec 20, 2019

From my understanding of the :ssl options used by the erlang module (which HTTPoison uses), the developer will have to write a custom middleware anyway to be able to read/reference key files . I have already updated the documentation under Basic Concepts in #31 with an example on how to write a custom middleware to add options to Crawly.Request (which are then passed to HTTPoison`)

I don't think merging this PR is necessary, just maybe need to add even more to the docs?

@Ziinc
Copy link
Collaborator

Ziinc commented Dec 20, 2019

I think the points raised in :

would be better to tackle, as this PR is not necessary since options can already be set through the request.
@oltarasenko close if you agree, thanks

@Ziinc
Copy link
Collaborator

Ziinc commented Dec 28, 2019

Closing as documentation on how to pass HTTPoison request options has been added into documentation in #31

@Ziinc Ziinc closed this Dec 28, 2019
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

2 participants