alexdlaird / pyngrok

A Python wrapper for ngrok
https://pyngrok.readthedocs.io
MIT License
421 stars 59 forks source link

Return an https URL when option `bind_tls` is enabled #63

Closed alfozan closed 4 years ago

alfozan commented 4 years ago

Description Fix bug reported in https://github.com/alexdlaird/pyngrok/issues/62

Issues https://github.com/alexdlaird/pyngrok/issues/62

Test plan:

from pyngrok import ngrok
print(ngrok.connect(port="5000", proto="http", options={"bind_tls": True}))

output: https://X.ngrok.io prefix is https as expected

other use cases are not affected:

from pyngrok import ngrok
print(ngrok.connect(port="5000", proto="http"))

output: http://X.ngrok.io

from pyngrok import ngrok
print(ngrok.connect(port="5000", proto="http", options={"bind_tls": False}))

output: http://X.ngrok.io

All outputs are as expected

Environment:

PYNGROK VERSION: 4.1.9
NGROK VERSION: 2.3.35

Tested on: https://colab.research.google.com/

Type of Change

Checklist

codecov[bot] commented 4 years ago

Codecov Report

Merging #63 into master will decrease coverage by 1.22%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   95.51%   94.29%   -1.23%     
==========================================
  Files           5        5              
  Lines         491      491              
==========================================
- Hits          469      463       -6     
- Misses         22       28       +6     
Impacted Files Coverage Δ
pyngrok/ngrok.py 92.91% <100.00%> (-0.79%) :arrow_down:
pyngrok/process.py 92.34% <0.00%> (-2.56%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fb26bfb...b839fb7. Read the comment docs.

alexdlaird commented 4 years ago

Trying to remember if there was a reason I specifically didn't do this ... can't think of anything at present.

Could you add a unit test to assert the public_url is properly asserted as https? There should be similar ones you can base it off of. Thanks!