dandi / redirector

Apache License 2.0
0 stars 2 forks source link

Add GIRDER_LOCAL_URL environment variable #25

Closed jwodder closed 4 years ago

jwodder commented 4 years ago

Needed for dandi/dandi-cli#186

satra commented 4 years ago

@jwodder - is this a make my life easier PR for dandi-cli or do you actually need access to both instances simultaneously.

i can easily imagine:

SET GIRDER URL 1: run tests SET GIRDER URL 2: run tests ... unless there is a test that needs both URLs simultaneously.

jwodder commented 4 years ago

It's for accessing the same instance of Girder through different URLs. dandi-cli's tests involve a Docker Compose setup with a Girder container and a redirector container. The redirector container has to access Girder via the url http://girder:8080, yet responses from /server-info have to use the URL http://localhost:8081. Neither URL is valid in the other one's context, so we need a way to tell serve.py to use one URL when accessing Girder and another when reporting Girder's URL via /server-info.

yarikoptic commented 4 years ago

a possible "freshly discovered" alternative which might work is a host mode for network (yet to be tested to work elsewhere but my laptop) but then ideally we would need to be able to tune up the port of redirector instead of hardcoded popular 8080... might be worth doing that (adding e.g. DANDI_REDIRECTOR_PORT) anyways I guess... then we hopefully would stop pestering redirector for a longer period of time ;-)

edit: may be could be REDIRECTOR_URL and then code would parse out possible port... redirector ATM does not reveal its own URL, and I hope it would never need to, but might be worth getting ready for that? might want to handle possible https:// protocol spec? sorry for piling up.... I am happy with any solution which would get us forward

yarikoptic commented 4 years ago

@satra ping. this PR is waiting on your blessing or cursing... AFAIK we found no alternative insofar and would need it to establish more realistic testing within dandi-cli

satra commented 4 years ago

@yarikoptic - i'll merge this for now, but let's see if we can figure out a better solution.