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

[#82] Adds proxy as a flag and config option #121

Merged
merged 9 commits into from
Oct 7, 2022

Conversation

MitchellBerend
Copy link
Collaborator

Resolves #82

This pr adds a proxy flag and a proxy option to the tool.toml, if both are supplied the proxy flag supersedes the config option.

Additional tasks

  • Documentation for changes provided/changed
  • Tests added
  • Updated CHANGELOG.md

@MitchellBerend
Copy link
Collaborator Author

@chshersh @maku can you both check this out? This implementation functionally works for me, but I don't see a way to actually test this.

@chshersh chshersh added enhancement New feature or request hacktoberfest-accepted https://hacktoberfest.com/participation/ labels Sep 30, 2022
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

I don't use a proxy so I can't verify whether this works or not 😞

If anyone who uses proxy can test if this really does the job, it would be much appreciated!

cc @maku

Comment on lines +43 to +44
/// Proxy which will get used for all communication
pub proxy: Option<ureq::Proxy>,
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a particular reason we may need a different proxy for individual assets?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pub fn prefetch(tools: BTreeMap<String, ConfigAsset>) -> Vec<ToolAsset> {
let total_count = tools.len();
let prefetch_progress = PrefetchProgress::new(total_count);
prefetch_progress.update_message(0);
let tool_assets: Vec<ToolAsset> = tools
.iter()
.enumerate()
.filter_map(|(index, (tool_name, config_asset))| {
prefetch_tool(tool_name, config_asset, &prefetch_progress, index)
})
.collect();
prefetch_progress.finish();
let estimated_download_size: u64 = tool_assets.iter().map(|ta| ta.asset.size).sum();
let size = HumanBytes(estimated_download_size);
eprintln!(
"{emoji} Estimated total download size: {size}",
emoji = PACKAGE,
size = size
);
tool_assets
}

This method makes the first call to the github api. It only has access to the names and ConfigAsset struct.

The implementation of this was kind of messy so there could be a better way to implement this so the Client gets recycled for each api call instead of a new instance being created for every tool.

src/config/toml.rs Outdated Show resolved Hide resolved
src/sync/install.rs Show resolved Hide resolved
@chshersh chshersh added the help wanted Extra attention is needed label Sep 30, 2022
@chshersh
Copy link
Owner

chshersh commented Oct 1, 2022

Thanks for working on this, @MitchellBerend! 👏🏻

Just a heads-up, I have more suggestions about changing the code. But before putting more work into this, I'd like to have a confirmation that the feature works. I don't use any proxy and once this feature is merged, it will probably remain in the code forever and require maintenance.

Spending more of my free time maintaining features I don't even use is not something I'm eager to do 😌
And it's even worse if nobody else is going to use them either 😢

@maku
Copy link

maku commented Oct 3, 2022

@MitchellBerend @chshersh sorry for delay, recently I worked mostly remote where I do not have a proxy server. But today I tried the implementation (I cloned https://github.com/MitchellBerend/tool-sync).

For my use case it worked perfectly. (I am using a proxy without auth...)

@chshersh
Copy link
Owner

chshersh commented Oct 3, 2022

@maku Thanks for checking the implementation!

In that case, I'm happy to accept the patch 🙂 I'll get around providing more feedback on the implementation once I have time 👌🏻

@MitchellBerend
Copy link
Collaborator Author

@maku what os are you on? I would also like to know if this works on windows.

Should auth not be supported on this? My use case also does not require auth so I did not implement it. Support for this can always be added later when needed so maybe this is work that doesn't need to be done right now.

@maku
Copy link

maku commented Oct 3, 2022

@MitchellBerend I tried it on windows

@MitchellBerend MitchellBerend marked this pull request as ready for review October 3, 2022 18:21
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Okay, I'm happy to merge it because it works 👍🏻

I'll create a separate issue to improve the implementation

@chshersh chshersh merged commit 4f878ea into chshersh:main Oct 7, 2022
@chshersh
Copy link
Owner

chshersh commented Oct 7, 2022

And here is the issue with the future improvements suggestions:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest-accepted https://hacktoberfest.com/participation/ help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

does not work behind a proxy (windows)
3 participants