alexdlaird / pyngrok

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

Add Support for Labels / Cloud Edge #117

Closed alexdlaird closed 9 months ago

alexdlaird commented 10 months ago

Description Add support for labels in tunnel definitions. This does not add fully parity support for ngrok's new Cloud Edge feature set, but it does allow its basic functionality to be used with pyngrok now.

Issues

110, #113, #116

Type of Change

Testing Done A clear and concise description of the new tests added to validate the change as well as any manual testing done.

Checklist

mapacheverdugo commented 10 months ago

You also need to remove schemes parameter when is a labeled tunnel

alexdlaird commented 10 months ago

You also need to remove schemes parameter when is a labeled tunnel

I couldn't find a scenario where this was the case, could you provide me with an example where the tests fail for this?

Note that you can't pass labels to connect, that'll never work due to how the ngrok API works. This functionality will only work through tunnel_definitions (i.e. setting it in the ngrok config file and starting that tunnel via its name).

mapacheverdugo commented 10 months ago

I couldn't find a scenario where this was the case, could you provide me with an example where the tests fail for this?

http_tunnel = ngrok.connect(
    addr="http://localhost:3000",
    bind_tls=True,
    labels= ["edge=edghts_example"]
)
alexdlaird commented 10 months ago

I couldn't find a scenario where this was the case, could you provide me with an example where the tests fail for this?

http_tunnel = ngrok.connect(
    addr="http://localhost:3000",
    bind_tls=True,
    labels= ["edge=edghts_example"]
)

Ah, I see, bind_tls is a legacy ngrok v2 param. It upconverts to schemes, but in either case, that param is incompatible with labels. The solution is never pass either bind_tls or schemes to connect() when you are using labels. I'd rather ngrok continue to throw the error if that, proto, or other incompatible params are passed (like a Bad Request).

codecov[bot] commented 10 months ago

Codecov Report

Patch coverage: 86.48% and project coverage change: -0.45% :warning:

Comparison is base (356c05a) 93.32% compared to head (04aaef2) 92.87%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #117 +/- ## ========================================== - Coverage 93.32% 92.87% -0.45% ========================================== Files 5 5 Lines 599 632 +33 ========================================== + Hits 559 587 +28 - Misses 40 45 +5 ``` | [Files Changed](https://app.codecov.io/gh/alexdlaird/pyngrok/pull/117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Laird) | Coverage Δ | | |---|---|---| | [pyngrok/ngrok.py](https://app.codecov.io/gh/alexdlaird/pyngrok/pull/117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Laird#diff-cHluZ3Jvay9uZ3Jvay5weQ==) | `87.21% <85.71%> (-0.49%)` | :arrow_down: | | [pyngrok/conf.py](https://app.codecov.io/gh/alexdlaird/pyngrok/pull/117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Laird#diff-cHluZ3Jvay9jb25mLnB5) | `96.66% <100.00%> (+0.11%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alexdlaird commented 10 months ago

As this PR currently stands is how I'm proposing we implement this. Going to sit on it for a day or two while I implement similar support in java-ngrok, then I'll merge this. You'll see basic support for ngrok's Cloud Edge then in pyngrok 6.1.0.

alexdlaird commented 9 months ago

Sister PR in java-ngrok: https://github.com/alexdlaird/java-ngrok/pull/76