-
Notifications
You must be signed in to change notification settings - Fork 75
Add support for 2.0 apigw request/response #30
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
Conversation
awesome I'll try to review soon! |
@tj I meant to drop a note here, I have been using it for an API keen to get your thoughts on supporting both v1 and v2 in the same library or making a new one? The main area i am finding clunky is the Context retrieval part, as both v1 and v2 are very different structures. Keen to get your thoughts on this, will fix the build issue tonight. Thanks! |
Shouldn't Edit: Pragmatic example: https://github.com/googleapis/gax-go |
@wolfeidau Yeah seems like a v2 dir or something is the way to go, I'm not too familiar with the idioms there yet Go's versioning is a bit of a mess once you're above v1. Happy to add you as a contributor if you want! I don't have too much time for this package at the moment, but I appreciate the contrib! |
Pretty cool. I was just about to work on a PR for this myself. That said - it feels like having both versions in a single API has little benefits. |
Any suggestions on how to proceed with this? @tj @wolfeidau @piotrkubisa |
@tj I would be happy to help out, I have been using this library for quite some time. I think the idea of operating a v2 branch is a good one, I am however looking around for some good references as it is quite a new thing in Go. I can update the PR based on that v2 suggestion, it looks like a well documented approach. We then just need to tag v1.1.2, for base module and v2.0.0 for the sub module by the looks of it. |
Awesome! Sent you an invite for access to the repo. I can try to still take quick glances at pull-request but feel free to do what you think is best there, I should learn this stuff too 😆 bound to hit 2.0 on other packages at some point |
Sounds great. @wolfeidau let me know if I can be of any help. |
FWIW: I was hanging out on on the go channel on IRC. The directory approach seems to be the old way for very old go get clients. These days people seem to use just branches. So unless there is the a need for supporting older versions of go, just adding a v2 branch might be the cleaner way to go. |
Yeah I have seen the branch model, just wondering if managing PRs across two different branches would be a hassle given that both V1 and V2 gateway versions may need similar patches, or updates. Yeah I am intrigued how people manage branches. It feels like this is a bit of a quandary, both formats could take advantage of updates in the core libraries http package, an example of this is the recent header changes golang/go#34799 So i know we are talking about v1 and v2 of these protocol, and separating at least at the folder level seems wise, but would it be more complex to maintain two branches? |
I honestly cannot say. It probably heavily depends on the amount and size of changes to come. Looking at the commit history it does not seem to be much additional maintenance work with either approach. I'd only try to avoid having the versions in the struct names but rather use modules for that. That should be covered by both. Other than that I personally find the branching model cleaner - but I have no strong feelings either way. |
Hmm yeah maybe you're right, a directory might make more sense in this case since we're talking about two versions of an AWS API, not two versions of this package. The API Gateway stuff is effectively two separate products, I don't think v1 is deprecated or anything, just different? |
v1 feels slightly deprecated but there is no such statement that I am aware of - and surely it isn't going away anytime soon. If the goal is to support both versions, just using a sub package per version seems the way to go (to me). |
I pushed up a version separated into two modules, I actually did this during the outage yesterday and the PR didn't update 😓 So a couple of things to note:
The byproduct of being separate means merging this won't effect existing developers 🤞 Would be good to switch this repo to GitHub actions, if there are no qualms I can add a workflow based on https://github.com/apex/rpc/blob/master/.github/workflows/test.yml and get it to test both modules. |
So - merge and then work on the GitHub actions? or what's the path forward? |
* This adds a new v2 module in a subfolder * Ensure tests use canonical format used in 1.14 * Upgrade dependencies * Use the Handler interface to add some extensibility
I have reverted most of the changes to the original package, added some test coverage and updated commits. Not sure if @tj has time to take a look, either way I will 🚢 it tomorrow. |
Uh oh!
There was an error while loading. Please reload this page.