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

Improve DU download #24

Closed
wants to merge 3 commits into from
Closed

Improve DU download #24

wants to merge 3 commits into from

Conversation

mubaidr
Copy link
Contributor

@mubaidr mubaidr commented May 30, 2023

This pull request adds

  • proxy support for user environment proxy configuration.
  • checks if exe already downloaded then skip download

@mubaidr mubaidr changed the title Add proxy support for DU download Improve DU download May 30, 2023
@simoneb
Copy link
Owner

simoneb commented May 30, 2023

Thanks for the PR, but I'm not too keen to go ahead with the current implementation because it feels very tied to your specific environment, so ideally I would like to come up with something more generic.

First of all let me ask this question: in the issue you commented that you needed to be able to use a local file system path, so I would like to understand if the requirement changed now.

@mubaidr
Copy link
Contributor Author

mubaidr commented May 30, 2023

Can you explain what do you mean by tied to specific environment?

For what I understand, anyone using system wide proxy server, it is recommended or optimal way to set environmental variables. AFAIK

First of all let me ask this question: in the issue you commented that you needed to be able to use a local file system path, so I would like to understand if the requirement changed now.

Requirements still exists.

@mubaidr mubaidr closed this May 30, 2023
@simoneb
Copy link
Owner

simoneb commented May 30, 2023

I'm not sure why you closed this, I think the request makes sense, I just wanted to come up with a more generic way to handle this

@mubaidr
Copy link
Contributor Author

mubaidr commented May 30, 2023

Please share idea, I will open new pull request.

@simoneb
Copy link
Owner

simoneb commented May 30, 2023

If your requirement is to be able to load the file from the file system, can't we just handle it in the existing environment variable and, if it's a url, download it, otherwise copy the file from the file system to the expected location (or even just use it as-is)?

@mubaidr
Copy link
Contributor Author

mubaidr commented May 30, 2023

These are two different requirements:

  • To be able to specify already downloaded file (file:// protocol) and yes, this will be handled by the existing environment variable i.e. FAST_FOLDER_SIZE_DU_ZIP_LOCATION
  • To be able to use this tool behind proxy servers. We need to read proxy settings from user environment variables.

Sorry for the confusion

@simoneb
Copy link
Owner

simoneb commented May 30, 2023

Okay fair enough. I'm happy to get a PR which does either or both. When possible, please consider including unit tests to check that this works

@mubaidr mubaidr mentioned this pull request May 31, 2023
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.

2 participants