search

captaincolonelfox / TeleTok

Telegram bot to download TikTok videos
https://t.me/TeleTockerBot
MIT License
48 stars 48 forks source link

Feature/proxy #29

Closed arslan-charyyev closed 3 months ago

arslan-charyyev commented 3 months ago

This PR adds support for specifying a proxy URL via an environment variable PROXY. A new dependency aiohttp-socks had to be added to support the proxy feature.

Additionally, the aiogram library's minor version was bumped for maintenance purposes.

Finally, this PR includes a GitHub Actions workflow that runs the code quality checks for each PR to main branch. To my surprise, it was executed for this PR already, even though it hasn't been merged in the upstream yet.

captaincolonelfox commented 3 months ago

Hi @arslan-charyyev

CI pipeline looks great, but it would be better to create it as a separate PR, so it can be reviewed and merged independently

And as I can see, proxy only used in aiogram.Bot instance. I’m not sure, if I understand the problem it will solve, can you explain it a little bit, why we need that?

arslan-charyyev commented 3 months ago

Hi. To be honest, the CI is just 1 small file, so I don't wish to go through the process of creating and discussing another PR. I would rather focus on implementing features for now. I will just remove it then.

The proxy solves the problem when an internet traffic has to for whatever reason go through some proxy server (for example there is no ability to send network requests directly). Apart from aiogram it is used in the http client to make requests.

captaincolonelfox commented 3 months ago

Oh yeah, my bad, I somehow overlooked that it was used in httpx client as well. Then, of course, it make sense, but I just want to make sure, that it is useful feature, because it will requires maintaince support. Maybe it would be better to leave it out of scope for this app, and let the hoster of the bot to solve it by routing traffic through proxy on os/router level

About CI - I find it useful, so I will just cherry-pick the commit to main, so no worries about additional work from your side

arslan-charyyev commented 3 months ago

let the hoster of the bot to solve it by routing traffic through proxy on os/router level

This is easier said than done, and I for one found it hard to do when running the app natively for debugging purposes. But at any rate, seeing as there is no enthusiasm for this feature, I will just close this PR.

captaincolonelfox commented 3 months ago

Oh, sure. Don’t get me wrong, it’s not about enthusiasm, I was just trying to figure out, what was initial intent for this PR. I don’t want to include any code for “just in case” scenarios, because it increase maintain burden: we must keep it updated and tested. And if we can move it to another layer or solve the problem by using different tool - that’s ideal situation for us

arslan-charyyev commented 3 months ago

I don’t want to include any code for “just in case” scenario

Well, it was a must for me in order to be able to run the project locally. Do you know of a simple way to run the project on Windows machine, while routing the entire app traffic through a proxy? That would require some serious intervention on the networks layer of the OS, probably with some custom network adapter. It is at least 10x more work than adding 2 lines of config parameters... The proxy configuration options in virtually all network libraries exists for a reason, and people such as myself do benefit from them tremendously. The intent behind adding proxy options is self-evident - to easily route the app traffic whenever it is necessary, such as in situations when direct connection is not possible.

captaincolonelfox commented 3 months ago

Ok, you see, I didn't have any goal to upset you. While I know why proxy exists and how to use it, what I was trying to do is to better understand your intention for this PR. You didn't mention what goal you try to accomplish by adding proxy in a description of this PR - you just list what you added. The goal of that was not self-evident. And now I can see, that you tried to add proxy for your development environment, so you can run project locally. And that's totally fine. But then I want to highlight the issue I was trying to communicate - when you expose a PROXY, someone will use it and will depend on it. You now need to keep it working, support different proxy servers and protocols and solve issues, related to that. Someone will come to you and ask to fix proxy issue for their specific problem. I would like to keep a scope of this project small. And in your case it make sense only to support it for local development, so probably it would be better to export this env as "DEV_PROXY". And still, I would prefer to solve development environment issues with different tools