regro / conda-metachannel

like a conda-metapackage but for channels!
MIT License
69 stars 4 forks source link

Add url argument for conda channel mirrors #21

Closed quartox closed 5 years ago

quartox commented 5 years ago

Would like to talk about url argument name (I think there is probably a better name) and what testing needs to be done.

I realized the use case is more complicated than I first thought. Our mirror follows the pattern http://internal.url.com/conda-forge-remote so this PR allows python app.py --url https://internal.url.com and then appends channel and arch or python app.py --url https://internal.url.com/{channel}-remote/ or python app.py --url https://internal.url.com/{channel}-remote/{arch} in case the channel and/or arch are embedded in the middle of the url.

I have manually proven that this will work on linux using the following:

$ python --url https://internal.url.com/{channel}-remote/

in another terminal

$ conda create --name metachannel-test python=3.7 pandas -c http://127.0.0.1:20124/conda-forge/pandas

scopatz commented 5 years ago

Maybe call it base_url?

scopatz commented 5 years ago

Or if you just wanted internal.url.com, and not http://internal.url.com, you could call it tld or domain

quartox commented 5 years ago

I like base_url since it can be a url pattern in the case where the user needs to include {channel}. In my use case I also need to pass http://internal.company.com/artifactory/{channel}-remote so I think domain or tld might be more confusing.

scopatz commented 5 years ago

Sounds good to me! Let us know when this is ready fro another review!

quartox commented 5 years ago

Ready for another review. I ended up putting the default in each function/class that gets passed the base_url but that may have been a misunderstanding of your comment. I never know if I should have the default in each function or only certain ones, happy to change it to whatever you would prefer.

scopatz commented 5 years ago

default in each seems preferable.

scopatz commented 5 years ago

This LGTM @mariusvniekerk @CJ-Wright any thoughts?