-
Notifications
You must be signed in to change notification settings - Fork 97
v2 API #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v2 API #203
Conversation
12d562d to
17b7c1c
Compare
|
This is ready for review. A few remaining items I'd like to get through myself before merge:
|
63c7aa9 to
59a7df9
Compare
|
additional tests have been added as well as some codebase cleanups and small modifications to the API based on review and suggestions before merging it needs a rebase/squash and a git tag of v2.0.0 |
nijikokun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this feels really nice so far. Tests and formatting passed locally on macOS and Windows for me and no major issues in the code from what I read / tested. I had one note on one of the options.
Alice-Lilith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to @bmpngrok's nits, otherwise looks clean to me.
euank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new package is way simpler, and I overall like the UX!
The only wart I've found so far is that waiting for a session to shutdown (i.e. from clicking 'stop' in the dashboard) doesn't seem possible with this API. I could be missing something, and I'd be happy to know if I am
I also have a small set of code nits
Hmmm, that's true and I agree that it's a miss. On the other hand, it also was not possible with the previous version of the API. I'd like to advocate that we don't require that in this PR because this PR is already large enough and it can be safely added later without a breaking change. If that's amenable, I'll add it to the TODO |
|
I'm fairly certain this happens with v1 as well but this is my first time using the SDK so far so it was a fairly rough edge that would be great to address: if you don't have your authtoken env variable (or it's wrong), the SDK will just fail in the background and attempt to reconnect every 30 seconds and hang fover. I was only able to find out the issue by adding my own logger because it didn't even print it anywhere by default (I was running doing the connection in a |
indeed, that's this issue: #176 since it's not (meaningfully) API-breaking, i'd like to fix that post-merge to try to get this PR out without stacking on additional changes |
euank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another round of review!
I think the RPC bit needs another round of iteration, and I agree with ryno that the error code stuff should also be properly plumbed.
The Agent vs AgentSession also feels quite confusing, since a lot of the methods I'd expect to be on the AgentSession (like Disconnect) are actually on the Agent.
I get that the abstraction is "One agent has an automatically reconnecting session, so it can have multiple sessions over time meaning creating an endpoint should then re-create the endpoint, so it's tied to the agent not the session", but it still feels slightly off somehow.
bmpngrok
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have anything to add, so approval conditional on responding to Euan and Ryno's concerns
See CHANGELOG.md for details.
v2 API for ngrok-go
Summarized new API:
fixes #202 #193 #178