okfn / opendataeditor

No-code application to explore and publish all kinds of data: datasets, tables, charts, maps, stories, and more. Forever free and open source project powered by open standards and generative AI.
http://opendataeditor.okfn.org
MIT License
150 stars 18 forks source link

Detect proxy in desktop #396

Closed roll closed 2 days ago

roll commented 1 month ago

Please make sure that all the checks pass. Please add here any additional information regarding this pull request. It's highly recommended that you link this PR to an issue (please create one if it doesn't exist for this PR)

roll commented 1 month ago

@pdelboca @guergana Please take a look.

I don't yet have a proper env with proxy to test; going to do so on Monday

cloudflare-pages[bot] commented 1 month ago

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4ac59f7
Status: ✅  Deploy successful!
Preview URL: https://a2b7f170.opendataeditor.pages.dev
Branch Preview URL: https://359-detect-proxy.opendataeditor.pages.dev

View logs

roll commented 1 month ago

@pdelboca Good finding, I'll try it

roll commented 4 weeks ago

@pdelboca No, we can't use Electron native detector -- it doesn't provide credentials information (I guess for security reasons). So I guess it's only useful if network calls are made through electron itself.

@pdelboca @guergana I tested it, it works for me. Can you please try? I think we might need to make some adjustments later when we see it in the real world to cover hidden edge cases.

guergana commented 3 weeks ago

@pdelboca No, we can't use Electron native detector -- it doesn't provide credentials information (I guess for security reasons). So I guess it's only useful if network calls are made through electron itself.

@pdelboca @guergana I tested it, it works for me. Can you please try? I think we might need to make some adjustments later when we see it in the real world to cover hidden edge cases.

@roll which operating system did you test it with? Since this a desktop issue, I think we should try to test in Ubuntu, Windows 10 (version of the user who reported the issue) and Mac.

roll commented 3 weeks ago

@guergana Currently, it is tested in Ubuntu.

I think there is a gotcha with the get-proxy-settings library as it will definitely won't work in each and every case. My hope that at least it will cover the most of them. In next iteration, we additionaly might need to add an UI for setting up a proxy I suspect

guergana commented 3 weeks ago

@roll how did you test the proxy? I tried configuring the default proxy and then tried to make the traffic be caught with postman and I couldn't make it work..

Could you list the steps that you followed? I think I missed something. I will also try to test it in my virtual machine for windows.

roll commented 3 weeks ago

@guergana I enabled a proxy in Ubuntu and then ensured that pip and python calls were done using it in the env vars (in the electron logs)

guergana commented 1 week ago

Hello, @roll I am testing the branch in windows 10 pro (with a virtual machine in Ubuntu) and i got the error with the proxy activated:

I am not sure the issue is fixed for Windows 10 (the original operating system of the person who opened the issue).

image

Steps to reproduce the error in Windows 10:

There is nothing related to this error in the logs c:/user/.opendataeditor/logger/main.log

Note: the port 8080 is not set in the address i tested with, I also tried with :80 which exists but is serving the static files :see_no_evil:

image

I probably need to find a proper proxy address to test with but I still don't understand why we need a proxy upon loading the app, shouldn't all the packages already be contained in the installation package instead of downloading them on startup?

roll commented 1 week ago

I probably need to find a proper proxy address to test with but I still don't understand why we need a proxy upon loading the app, shouldn't all the packages already be contained in the installation package instead of downloading them on startup?

The problem that packaging the python dependencies with tools like py2exe doesn't really work. We tried a few times and never got any reliable results. Of course, it was pretty time-limited attempts maybe with proper timeline it would be possible to achieve.

The second thing is accessing remote files via the server -- it also requires proxy information. It can be eliminated if fetchFile endpoint is implemented directly in Electron though

guergana commented 1 week ago

I probably need to find a proper proxy address to test with but I still don't understand why we need a proxy upon loading the app, shouldn't all the packages already be contained in the installation package instead of downloading them on startup?

The problem that packaging the python dependencies with tools like py2exe doesn't really work. We tried a few times and never got any reliable results. Of course, it was pretty time-limited attempts maybe with proper timeline it would be possible to achieve.

The second thing is accessing remote files via the server -- it also requires proxy information. It can be eliminated if fetchFile endpoint is implemented directly in Electron though

Since we have more developer time available now probably than in the previous iteration maybe we can try to implement the self contained version? @roll . What do you think about this @pdelboca ?

pdelboca commented 1 week ago

@guergana I agree with both of you.

When working on #427 I read a little bit about how to package Python applications and as @roll mentioned, sadly all bundling solutions for python do not work as expected.

I do think that we should bundle it since you are right that it doesn't make sense to have errors or need internet connection to open the application. Maybe instead of an executable file we zip/unzip the virtual environment on build/install.

I'll take a look and propose some ideas.

roll commented 2 days ago

CLOSED in favour of #446

(parts of this PR can be used to handle #449)