-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: add context.Context support to Go SDK #301
base: main
Are you sure you want to change the base?
feat: add context.Context support to Go SDK #301
Conversation
50217bf
to
12c8e79
Compare
@natebrennand: Thanks for the PR! Haven't tested this locally yet, but could you look at what the test failures are about? |
It's kinda hard to get the failures to surface. Ultimately accessed it via the docker logs.
Looks like we'll need to land the first of two commits in 197 that adds new methods to the client: https://github.com/twilio/twilio-go/pull/197/commits I'll separate that PR. |
@rakatyal, twilio/twilio-go#196 has the changes to the underlying client that will be needed before this PR's tests will pass. |
d1fb508
to
228615e
Compare
@childish-sambino, I did a rebase on this PR to update w/ master and try to re-prompt the CI so I removed the merge commit. I think you may need to re-enqueue the build? |
228615e
to
2ec6804
Compare
Sorry, iteration speed is slow here because it's a bit obtuse to access the test results. It seems I had to regenerate the some of the examples here too. I've amended the last commit to include these. |
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.
Nice and clean. Thanks!
@childish-sambino, if that diff is necessary on the tests, I think my statements on #196 may have been incorrect. If this test client needs to be amended to get the tests to work, things are busted. When I ran the tests locally without your added commit I got failures in
I cannot tell if this is related to the docker-compose setup or what exactly. |
@natebrennand The error you're seeing is the result of not being linked up with the prism servers defined in the docker-compose file. The failing tests are attempting to connect to the prism instance to verify expected behaviors. The reason why they're failing even when run with docker-compose is that this method is not being called to swap the scheme from https to http: TestClient.getParsedUrl The |
FYI, I'm going to revert the changes to twilio-go until this is resolved so they don't affect the next release. |
Hi, any news for this? |
Update: Internal Backlog Reference (DI-2543) |
It's nearly two months since the last update - can you give some insight on when this feature may be available? |
Hi, do you have any updates on this? |
Fixes twilio-go#98
This PR updates the mustache templates that generate the Go client to leverage the context request methods that will be introduced in: twilio/twilio-go#196
The newly generated SDK is available at twilio/twilio-go#197.
Checklist
make test-docker
python examples/build_twilio_go.py path/to/twilio-oai/spec/yaml path/to/twilio-go
and inspect the diffmake test
intwilio-go
twilio-go