googlearchive / cloud-functions-go

Unofficial Native Go Runtime for Google Cloud Functions
Apache License 2.0
423 stars 44 forks source link

Add preliminary pubsub and bucket trigger support #29

Closed ssttevee closed 6 years ago

ssttevee commented 6 years ago

This PR is not necessarily ready to be merged, I'm more looking for some comments and discussion.

To handle a pubsub or bucket trigger, use nodego.UseEventHandler() instead of http.Handle():

nodego.UseEventHandler(func(event *nodego.FunctionEvent) error {
    msg, err := event.PubSubMessage()
    if err != nil {
        return err
    }

    // OR

    evt, err := event.StorageEvent()
    if err != nil {
        return err
    }

    return nil
})
ssttevee commented 6 years ago

I'm uncertain about StorageEvent fields, whether or not it should be included since I probably haven't included all the possible json object properties.

iangudger commented 6 years ago

I think this is the official parsing struct for StorageEvent: https://github.com/google/google-api-go-client/blob/master/storage/v1/storage-gen.go#L1224

ssttevee commented 6 years ago

Do you think I should import it and use it directly even though it includes other unused fields? Likewise with the PubSubMessage: https://github.com/google/google-api-go-client/blob/master/pubsub/v1/pubsub-gen.go#L730

iangudger commented 6 years ago

That would probably be best.

iangudger commented 6 years ago

Any progress?

ssttevee commented 6 years ago

It seems to have been working fine on my own functions for now. Probably okay to be merged.

iangudger commented 6 years ago

Do you want to do the cleanup we discussed?

ssttevee commented 6 years ago

Do you not see the changes? I'm pretty sure they have been pushed.

ssttevee commented 6 years ago

There's one issue I've noticed with this PR that I'm still pondering.

The testing feature on the web ui (as well as gcloud beta functions call) does not work when using a non-http hook because that calls the /execute route where as the /execute/ route is being used. This works in js because express takes the /execute* route which does not work with the std http mux.

iangudger commented 6 years ago

With regard to the route, maybe we should just change the user code listener to accept /.

I think it would be better to put the new examples in the examples directory instead of examples/triggers.

googlebot commented 6 years ago

So there's good news and bad news.

:thumbsup: The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

:confused: The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot commented 6 years ago

CLAs look good, thanks!

ssttevee commented 6 years ago

Sorry, messed up some git commands...

I added support for the new event context: https://cloud.google.com/functions/docs/writing/background

ssttevee commented 6 years ago

I'm not sure if the nodego import from the events package should be an absolute or relative import. Depends if the intended use case is to import this package or to use this package as a starting point.