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

No way to set custom http.Client #63

Closed
broady opened this Issue Jun 15, 2016 · 20 comments

Comments

Projects
None yet
7 participants
@broady

broady commented Jun 15, 2016

@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jun 16, 2016

Member

@broady,

It looks like I might not need your pull request (we are still trying to work through the CLA issue) because we already have this pull request: #59

Would that get you what you need?

Member

thinkingserious commented Jun 16, 2016

@broady,

It looks like I might not need your pull request (we are still trying to work through the CLA issue) because we already have this pull request: #59

Would that get you what you need?

@broady

This comment has been minimized.

Show comment
Hide comment
@broady

broady Jun 16, 2016

@thinkingserious well, nope - that PR doesn't merge with master. But something like that, yes! :)

broady commented Jun 16, 2016

@thinkingserious well, nope - that PR doesn't merge with master. But something like that, yes! :)

@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jun 16, 2016

Member

@broady

Hehe, cool. I'll go ahead and get it updated to work in this version of the library and I'll ping you when its done. Thank you for being so responsive!

Member

thinkingserious commented Jun 16, 2016

@broady

Hehe, cool. I'll go ahead and get it updated to work in this version of the library and I'll ping you when its done. Thank you for being so responsive!

broady added a commit to GoogleCloudPlatform/golang-samples that referenced this issue Jun 17, 2016

broady added a commit to GoogleCloudPlatform/golang-samples that referenced this issue Jun 17, 2016

@amaendeepm

This comment has been minimized.

Show comment
Hide comment
@amaendeepm

amaendeepm Jul 9, 2016

Then how to use in Google Appengine where custom client http overriding is must-to-have requirement ?

amaendeepm commented Jul 9, 2016

Then how to use in Google Appengine where custom client http overriding is must-to-have requirement ?

@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jul 9, 2016

Member

@amaendeepm,

The fix is on our backlog and I've added your vote to the ticket, helping it to rise higher in the queue.

Member

thinkingserious commented Jul 9, 2016

@amaendeepm,

The fix is on our backlog and I've added your vote to the ticket, helping it to rise higher in the queue.

@sridharv

This comment has been minimized.

Show comment
Hide comment
@sridharv

sridharv Jul 13, 2016

For anyone who runs into this issue, I'm currently using the following function as a workaround. This doesn't require editing the existing sendgrid library.

import (
    "github.com/sendgrid/rest"
)

func api(client *http.Client, request rest.Request) (*rest.Response, error) {
    if len(request.QueryParams) != 0 {
        request.BaseURL = rest.AddQueryParameters(request.BaseURL, request.QueryParams)
    }

    req, err := rest.BuildRequestObject(request)
    if err != nil {
        return nil, err
    }

    res, err := client.Do(req)
    if err != nil {
        return nil, err
    }

    response, err := rest.BuildResponse(res)
    if err != nil {
        return nil, err
    }

    return response, nil
}

sridharv commented Jul 13, 2016

For anyone who runs into this issue, I'm currently using the following function as a workaround. This doesn't require editing the existing sendgrid library.

import (
    "github.com/sendgrid/rest"
)

func api(client *http.Client, request rest.Request) (*rest.Response, error) {
    if len(request.QueryParams) != 0 {
        request.BaseURL = rest.AddQueryParameters(request.BaseURL, request.QueryParams)
    }

    req, err := rest.BuildRequestObject(request)
    if err != nil {
        return nil, err
    }

    res, err := client.Do(req)
    if err != nil {
        return nil, err
    }

    response, err := rest.BuildResponse(res)
    if err != nil {
        return nil, err
    }

    return response, nil
}
@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jul 13, 2016

Member

Awesome @sridharv!

Thanks for sharing the workaround!

Also, I've added your voice to the issue so that it can get prioritized higher.

Can you and @broady send me your T-shirt size and mailing address so that I may mail out some swag for your support?

Member

thinkingserious commented Jul 13, 2016

Awesome @sridharv!

Thanks for sharing the workaround!

Also, I've added your voice to the issue so that it can get prioritized higher.

Can you and @broady send me your T-shirt size and mailing address so that I may mail out some swag for your support?

@sridharv

This comment has been minimized.

Show comment
Hide comment
@sridharv

sridharv Jul 15, 2016

Thanks for the T-shirt offer :)

If this above code works I can send you a pull request to create a new function like so:

func APIWithClient(client *http.Client, request rest.Request) (*rest.Response, error) {
    ...
}

sridharv commented Jul 15, 2016

Thanks for the T-shirt offer :)

If this above code works I can send you a pull request to create a new function like so:

func APIWithClient(client *http.Client, request rest.Request) (*rest.Response, error) {
    ...
}
@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jul 18, 2016

Member

@sridharv,

A pull request would be awesome. It would need to be done here: https://github.com/sendgrid/rest and we would need a signed CLA: https://github.com/sendgrid/rest/blob/master/CONTRIBUTING.md#cla

Otherwise, I'll be manually merging this one: #59

Thanks!

Member

thinkingserious commented Jul 18, 2016

@sridharv,

A pull request would be awesome. It would need to be done here: https://github.com/sendgrid/rest and we would need a signed CLA: https://github.com/sendgrid/rest/blob/master/CONTRIBUTING.md#cla

Otherwise, I'll be manually merging this one: #59

Thanks!

@mjgaylord

This comment has been minimized.

Show comment
Hide comment
@mjgaylord

mjgaylord Jul 19, 2016

Just an up vote on this issue. We also need support for this on Google Appengine.

mjgaylord commented Jul 19, 2016

Just an up vote on this issue. We also need support for this on Google Appengine.

@amaendeepm

This comment has been minimized.

Show comment
Hide comment
@amaendeepm

amaendeepm Jul 19, 2016

We decided to leave Sendgrid and moved to Campaign Monitor instead now. They dont claim GO-SDK but better than having a buggy one :-(

amaendeepm commented Jul 19, 2016

We decided to leave Sendgrid and moved to Campaign Monitor instead now. They dont claim GO-SDK but better than having a buggy one :-(

@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jul 19, 2016

Member

@amaendeepm,

Sorry to see you go.

We'll be updating this thread when the issue is fixed in case you decide to come back.

Also, I'm adding your vote to the issue to help move it up the priority queue.

Member

thinkingserious commented Jul 19, 2016

@amaendeepm,

Sorry to see you go.

We'll be updating this thread when the issue is fixed in case you decide to come back.

Also, I'm adding your vote to the issue to help move it up the priority queue.

@tkrajina

This comment has been minimized.

Show comment
Hide comment
@tkrajina

tkrajina Jul 20, 2016

Add another upvote from me. Same situation (appengine, still using the old version, and can't upgrade to the new one).

It's kind of weird that this (relatively simple) issue takes so long to be fixed.

tkrajina commented Jul 20, 2016

Add another upvote from me. Same situation (appengine, still using the old version, and can't upgrade to the new one).

It's kind of weird that this (relatively simple) issue takes so long to be fixed.

@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jul 20, 2016

Member

@tkrajina,

Thanks for the vote! I'm itching to get this fixed too.

The size and complexity of the fix is only one of many aspects of the prioritization process. To get this ticket moved up our priority queue involves:

  1. Exactly what you just did, a comment or a +1
  2. A pull request with a signed CLA

Note that the v2 of our library is not deprecated, if you have an urgent need that is an option.

Member

thinkingserious commented Jul 20, 2016

@tkrajina,

Thanks for the vote! I'm itching to get this fixed too.

The size and complexity of the fix is only one of many aspects of the prioritization process. To get this ticket moved up our priority queue involves:

  1. Exactly what you just did, a comment or a +1
  2. A pull request with a signed CLA

Note that the v2 of our library is not deprecated, if you have an urgent need that is an option.

@sridharv

This comment has been minimized.

Show comment
Hide comment
@sridharv

sridharv Jul 23, 2016

I've sent pull request sendgrid/rest#8

I've also sent a CLA to the email address mentioned, but I did have to add a small restriction to the patent license section.

sridharv commented Jul 23, 2016

I've sent pull request sendgrid/rest#8

I've also sent a CLA to the email address mentioned, but I did have to add a small restriction to the patent license section.

@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jul 23, 2016

Member

Hi @sridharv,

I've added a vote to this issue to move it up the queue and your modification to the CLA has been passed along for review by our legal team. Thanks!

Member

thinkingserious commented Jul 23, 2016

Hi @sridharv,

I've added a vote to this issue to move it up the queue and your modification to the CLA has been passed along for review by our legal team. Thanks!

@wizardofjoz

This comment has been minimized.

Show comment
Hide comment
@wizardofjoz

wizardofjoz Jul 28, 2016

+1 compatibility with App Engine. Just upgraded to SendGrid v3 but also noticed there's no official way to change the http.Client

At the time of writing this comment (July 28, 2016) the App Engine documentation still references SendGrid v2

wizardofjoz commented Jul 28, 2016

+1 compatibility with App Engine. Just upgraded to SendGrid v3 but also noticed there's no official way to change the http.Client

At the time of writing this comment (July 28, 2016) the App Engine documentation still references SendGrid v2

@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious

thinkingserious Jul 28, 2016

Member

@wizardofjoz,

Thanks for the upvote!

This pull request should get us there later today: sendgrid/rest#9

Member

thinkingserious commented Jul 28, 2016

@wizardofjoz,

Thanks for the upvote!

This pull request should get us there later today: sendgrid/rest#9

@thinkingserious

This comment has been minimized.

Show comment
Hide comment
@thinkingserious
Member

thinkingserious commented Jul 28, 2016

#73

@amaendeepm

This comment has been minimized.

Show comment
Hide comment
@amaendeepm

amaendeepm Jul 29, 2016

@thinkingserious Thanks a bunch !!! Now after 21 days of using Campaign Monitor we are coming back to SendGrid. Sent you Linkedin invite :-)

amaendeepm commented Jul 29, 2016

@thinkingserious Thanks a bunch !!! Now after 21 days of using Campaign Monitor we are coming back to SendGrid. Sent you Linkedin invite :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment