DM-13768 Fix firefly_client uploads to work with server on https #10
Conversation
|
@SimonKrughoff Maybe now that this package is in |
* remove port argument * implement the firefly_version function * change default host to include http://
stargaser
force-pushed
the
tickets/DM-13768
branch
from
d51ba19
to
5351abf
Compare
|
@SimonKrughoff this PR is now ready for your review. I have made changes to the doc files. These were being manually deployed to display-firefly.lsst.io by @jonathansick. Now it would be more appropriate to slot the package docs into pipelines.lsst.io following the new procedure. I would prefer to do that in another ticket, and I ask that you build the current docs locally, or that I send you an html file that I have built for your review. |
|
...on the other hand, it's supposed to take only 15 minutes to update the doc subdirectory? |
| @@ -88,18 +88,18 @@ def __init__(self, display, verbose=False, host="localhost", port=8080, | |||
| global _fireflyClient | |||
| if not _fireflyClient: | |||
| try: | |||
| _fireflyClient = firefly_client.FireflyClient("%s:%d" % (host, port), | |||
| _fireflyClient = firefly_client.FireflyClient("%s" % host, | |||
| raise RuntimeError("Unable to connect websocket %s:%d: %s" % (host, port, e)) | ||
| if (host == "localhost"): | ||
| raise RuntimeError("Unable to connect websocket %s: %s" % (host, e)) | ||
| if (host[:9] == "localhost"): |
There was a problem hiding this comment.
This isn't ever going to evaluate to true now because the protocol is included in the host. I think it would be better done using urllib:
from urllib.parse import urlparse
parsed_host = urlparse(host)
if(parsed_host.hostname == "localhost"):
This will be both more future proof and readable.
doc/index.rst
Outdated
| @@ -69,7 +69,7 @@ The next lines will set up the display, using a public Firefly server. | |||
| import lsst.afw.display as afw_display | |||
| afw_display.setDefaultBackend('lsst.display.firefly') | |||
| display1 = afw_display.getDisplay(frame=1, | |||
| host='lsst-demo.ncsa.illinois.edu', port=80, | |||
| host='lsst-demo.ncsa.illinois.edu', | |||
There was a problem hiding this comment.
Doesn't this require that the protocol be part of the host string, or does it default to http?
There was a problem hiding this comment.
It defaults to http inside firefly_client.FireflyClient, so it should work...a quick test is not working for me though, so let me work that out.
There was a problem hiding this comment.
You are right! It either has to be http://lsst-demo.ncsa.illinois.edu or lsst-demo.ncsa.illinois.edu:80.
It turns out that https://lsst-demo.ncsa.illinois.edu also works, i.e. the Firefly server there is deployed on port 443. SUIT is discussing making https:// the default in firefly_client.
|
Regarding the docs. I think it's fine to shorten them. Doing auto deploy I don't know enough to really have an opinion. |
do a proper check of localhost before launching browser include http:// in the default for host
To make the
lsst.display.fireflybackend work more naturally with https:// servers:portargument from the constructorlsst_distrib, substantially shorten the "installing" section