Skip to content
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

Client for GAE Environment #31

Closed
wants to merge 5 commits into from
Closed

Client for GAE Environment #31

wants to merge 5 commits into from

Conversation

keighl
Copy link

@keighl keighl commented Jul 20, 2016

Hey @sideshow, thanks for this killer APNS package.

This fork adds a distinct client for using the package from within Google App Engine (classic). Annoyingly, an application can't make a general net.Conn, it has to use a special appengine/socket package.

I added the +build appengine flag so it's only available when used inside the GAE go-sdk.

Usage is like this...

import (
    "net/http"

    apns "github.com/sideshow/apns2"
    "github.com/sideshow/apns2/certificate"
    "google.golang.org/appengine"
)

var apnsClient *apns.GAEClient

func init() {
    // ...

    cert, _ := certificate.FromPemBytes([]byte("..."), "")
    apnsClient = apns.NewGAEClient(cert).Production()

    //...
}

func SomeHandler(w http.ResponseWriter, r *http.Request) {
    // ...

    ctx := appengine.NewContext(r)

    apnsClient.SetContext(ctx) // Gotta set a valid context

    res, err = apnsClient.Push(&apns.Notification{})

    // ...
}

Tests:

// Use the gae-sdk go wrapper
$ goapp test

}

// Production sets the Client to use the APNs production push endpoint.
func (c *GAEClient) Production() *GAEClient {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

receiver name c should be consistent with previous receiver name gclient for GAEClient

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 1b511dc on keighl:master into eacc6af on sideshow:master.

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling ba8a121 on keighl:master into eacc6af on sideshow:master.

@sideshow
Copy link
Owner

Awesome! Thanks for the PR. Any chance you can get the hound errors fixed and add notes on how to use to the Readme. Thanks

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 4ea5a6b on keighl:master into eacc6af on sideshow:master.

@keighl
Copy link
Author

keighl commented Jul 22, 2016

Hey @sideshow, those hound issues are resolved, and I added a GAE example to the Readme. Lemme know if you think it's too detailed

https://github.com/keighl/apns2/blob/498a59482d3ef45203f506193046caac6e9e2eba/README.md#app-engine

@benguild
Copy link

Is there a reason why this is still pending merge after 11 months?

@sideshow
Copy link
Owner

Hi @benguild, at the time whilst I could run it locally I was unsure how to run it on GAE to perform end to end testing, so didn't get it merged.

Since then, we have added a request context to apns2, so this code would have to be refactored slightly. I did try and refactor this myself, but got totally lost when trying to use the gcloud app engine tools with build errors and gave up.

I think this is a good addition to the library. If someone wants to refactor 1 and provide instructions on how to install and run/test in production GAE then I will merge.

1: Probably need a similar file to this and this but called client_appengine.go which builds for app engine arch.

@sideshow
Copy link
Owner

Closing this in light of the last comment.
@keighl, thanks again for working on this and apologies it never made it in.
As above if someone wants to refactor and provide instructions on how to install and run/test in production GAE then I will merge. Closing until then.

@sideshow sideshow closed this Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants