Correct module name for Go module support #712
Conversation
SamWhited
changed the title
|
@SamWhited Thanks a lot for starting the discussion on this and providing so much details! Just wanted to reply and flag that it would take a few days before we can investigate further but I did not want you to think we were ignoring this! |
|
Thanks for the effort here @SamWhited! I'm a little bummed that the upgrade solution is going to look so nasty for us. I guess we weren't part of the target demographic while the new system was being designed. Luckily, we have released scripts to help with this, but they're going to have to touch a lot of files which is going to make merge conflicts a lot more likely. I believe the reason that your CI build is failing is that only Go 1.11 has built in support for Go modules (and it's succeeding). The others don't know how to read the I'd ideally prefer to wait at least one more Go version before bringing all this in so that Go modules are available without Vgo on both officially supported versions of Go. |
|
I had thought a point release added it to the supported older releases, but maybe that's still on the table; I'll dig around for the CL tomorrow and double check. EDIT: from https://golang.org/cl/109340:
CI appears to not have the point release, which I didn't notice before. So updating that should fix things. I pushed a change that I think will make travis use the latest point releases, so we'll see. |
|
That did it; CI is now green. The backported modules support was present in 1.9.7+ and 1.10.3+, so it's been out for a few versions. |
|
@SamWhited Ah, nice one. I noticed that you pulled And regarding |
|
They are imported by the I could go either way; I just didn't see what benefit leaving the checksum file out had. |
|
@SamWhited In that case, would you mind if we changed the If you wouldn't mind, a couple more questions because you may know the answer off hand (and it'll save me some testing): do you know what the impact of this change will be for users on pre-1.11 versions of Go who don't have access to the new module system? I assume they'd have to upgrade to their latest point release in order to be able to use this package again? Similarly, do you know what the impact would be for a project using an alternative package manager like dep? I suspect that majority of our users haven't made the Vgo migration yet, and I want to make sure not to break people too early. (And thanks again for all the contributions here!) |
It should probably have a testing build flag then, or at least be internal (although in that case it would still be a dependency of the main project and contribute to the go.mod file).
Can you expand on what this means? To my knowledge having these dependencies in the go.mod file effectively just pins the version that the project will be built with (which is in some ways more important when running tests). It shouldn't change what's linked in to the final output. I'd actually generally suggest including test-only dependencies in go.mod too (though I'm not aware of what the Go team's advice is here; if anyone knows I'd love to see some discussion of this), but I couldn't run tests locally so I figured that could come later if the stripe developers wanted it. EDIT: actually, running
That's my understanding as well; there are ways around that (eg. copying the package into a v52 directory and then asking them to update their imports), but they felt like a more invasive way to adopt modules to me.
I can't think why it would have any problems as long as they were using a version of Go with modules support or the limited compatibility for packages in the point releases. If the tool itself was type checking the package as part of its dependency graph building process it might stumble over the versioned import statements if it hasn't updated to use |
|
Ping; see previous comment. |
|
Rebased for v54. |
|
Bumped for v55. Ping again on the question I asked in my last reply. |
|
Thanks Sam, and sorry about the non-responsiveness. I think this is the right way to go — I just need to update our internal release tooling to support the changes. |
|
Not to worry; thanks for your consideration of this PR. Let me know if there's anything else I can do to help. |
|
If it's still not possible to merge this, would it be possible to remove the Thanks. |
|
Thanks for that fix up @SamWhited. Alright, bringing this in! |
|
I appreciate you guys trying to keep up with changes in Go, but now this library is unusable for older Go versions. Perhaps add a notice for this in the readme? |
|
@jerbob92 basic, read-only, support for modules was backported to 1.10.3 (the earliest supported version of Go that's still receiving security updates), and to 1.9.7. If you're using anything older, I'd highly recommend that you think about upgrading to something that's still receiving stability and security patches. |
|
@SamWhited |
|
FYI it seems that this change breaks compatibility with dep. The error is: |
|
@mdestagnol in your files if you don't use the |
|
I just gave it a shot too with this simple program: package main
import (
"fmt"
"github.com/stripe/stripe-go"
"github.com/stripe/stripe-go/customer"
)
func main() {
stripe.Key = "sk_test_..."
c, err := customer.Get("cus_ELj9iKfDdhwLIR", nil)
if err != nil {
panic(err)
}
fmt.Printf("c = %+v\n", c)
}And running Remember that dep is just managing dependencies and placing them in |
|
@brandur-stripe : actually got the same result as you. But 55.8 is not the latest version including the /v55 changes. And I get the error I reported in the first place when I add the following constraint to the Gopkg.toml: EDIT: I think what happens is that dep looks for the latest working version. It first tries the 55.9 but falls back to 55.8 since 55.9 doesn't work properly. |
|
I thought this had been merged, but it looks like it's not quite in dep yet: golang/dep#1963 |
|
And unless something major happens it likely won't make it in that version of [1] golang/go#29639 |
|
Another option could be to have 2 branches of this repo: one with |
|
Oddly, this only appears to be choking on the ignored = ["github.com/stripe/stripe-go/v55/form"]
[[constraint]]
name = "github.com/stripe/stripe-go"
version = "55.9.0"EDIT: It appears to be this bug: golang/dep#351
I'm not sure where you get that from, the existing dep authors are still able to use the repo, new maintainers can be added, etc. so it doesn't really seem to matter where the repo lives, just that someone be interested in getting that patch over the finish line and getting it merged. By the end of that thread I was somewhat tired and may have skimmed over something important though. |
It's not only the
|
That's odd, I can't reproduce that. Either way, you just want |
This doesn't work either. I get tons of errors like this when I run unit tests:
|
What version of Go and dep are you using? And what exists in your Alternatively, do you have a minimal working example that can reproduce this problem? It's working fine for me, but maybe there's a version mismatch in the tooling we're using, or your real project is importing stripe differently from my dummy test project. |
|
I upgraded to |
|
I'm sorry, I still can't reproduce the problem with that version of dep and that version of Go (using a package in the Can you also confirm that you're imports don't include the |
|
@SamWhited : when I update stripe-go to 55.9 using the ignore statement you provided in Gopkg.toml ( For comparison, here is what I have in the Gopkg.lock when dep use 55.8: Pretty sure that's why we get all these errors when forcing the upgrade, and the reason why dep doesn't update to 55.9 in the first place. |
|
@SamWhited Hey! So my own little experiments with Dep confirm what @mdestagnol is saying in that although certain operations work, we run into trouble during others. It seems like we need to see golang/dep#1962 fixed over there before it'll be possible to have a package like ours that can support both Dep and Go modules cleanly. Sam B. (Dep's maintainer) works with me, and I talked to him about this — he wants to see module support into Dep, but is experiencing some fairly severe time constraints right now. I think it'll land at some point, but probably not in the short term. Given that usage of Dep is still so widespread, I think our best course of action right now is to revert module support in stripe-go to turn it back into a "dumb package" such that projects under the Go module system can still pull it in, but it won't be a first class citizen of the ecosystem. Hopefully within the next few months we can see some movement on that Dep issue, and then give it a little time to roll out more broadly. Once some grace time has elapsed, we revert the revert and bring your patch back in. I'll put a note in the README containing this context with links. Sorry about all the run around on this — I really wanted to see good Go module support here, but it doesn't seem that it's currently possible to support the two biggest packaging systems in Go well simultaneously. Does that sound okay to you? |
|
I certainly wish you wouldn't, because we've already moved off of dep, but I understand your decision. Please consider removing the I had forgotten Sam B. works with you; we've met a few times, at various Go events, the contributors summits, etc. Give him my regards and thank him for all his work on dep for me. |
|
Sorry we couldn't find a better way, probably the right thing to do for now. And hopefully go1.12 create some momentum in the community to move toward Modules when released next month. |
|
@brandur-stripe out of curiosity, what did the problem end up being? I still can't reproduce it. Is it just stripping out the subpackages? Is this something that could be fixed by turning off pruning for the package? |
@SamWhited Yeah, I wish I wouldn't too :) But unfortunately it seems to be the best compromise at the moment.
Yep, forgot to mention it, but I saw your comment from before and I think that's the right way to go. Just strip modules out completely.
Will do!
So I don't understand Dep well enough to be able to say exactly what's going on here, but I started out with a minimal package main
import (
"fmt"
"github.com/stripe/stripe-go"
"github.com/stripe/stripe-go/customer"
)
func main() {
stripe.Key = "sk_test_..."
c, err := customer.Get("cus_ELj9iKfDdhwLIR", nil)
if err != nil {
panic(err)
}
fmt.Printf("c = %+v\n", c)
}Then ran Changing my Adding If I remove Happy to help look into what's going on there, but in general: I think there's enough evidence of subtle incompatibilities here that I believe that @mdestagnol is running into hard-to-reason about problems that are probably related to this change. Even if we could get past those, there's a little too much manual
@mdestagnol Yeah, I don't think this will be out of |
Ahh yes, that would make sense. Go still thinks that Replacing the constraint with an override, and using ignored = ["github.com/stripe/stripe-go/v55*"]
[[override]]
name = "github.com/stripe/stripe-go"
version = "55.9.0"
[prune]
go-tests = true
unused-packages = true
[[prune.project]]
name = "github.com/stripe/stripe-go"
unused-packages = false |
You certainly have a better mental model of what Dep is up to than I do :) I gave this a shot, and And using them without gave me the same problem of incompatible packages: I believe there's probably a way of getting this working, but even so, I think the onus in new Dep configuration to get back onto something that works is a little too high. |
Not really; I've fought with trying to get it to handle odd dependencies at several past jobs, but it's always been mostly trial and error (but thanks).
Odd, I thought that was working for me too. I'm not sure what weird thing is different about my setup, but I suppose it makes sense that Dep wouldn't understand how to download the /v55 version. But I agree, figuring out how to configure dep even assuming we could get it working is probably too much to ask of people who are already using it. |
Reverts: #712 (And an earlier PR that added a `go.mod` file in the first place.) Unfortunately, there are still some fairly bad incompatibilities between Go modules and Dep for users still on the latter. For now, we're going to revert Go module support because there doesn't seem to be any easy way (i.e., anything that's not maintaining a fork) of doing a good job of both. Projects can still use the Go module system, but will have to pull this package in as a pre-module incompatible one. We are hoping to eventually see basic module awareness merged into Dep: golang/dep#1963 If/when that gets done, we'll revert this revert after a small grace period, and hopefully be back to a place where both packaging systems are well supported. I'll also modify the README to clarify the current situation in another follow up PR.
Reverts: #712 (And an earlier PR that added a `go.mod` file in the first place.) Unfortunately, there are still some fairly bad incompatibilities between Go modules and Dep for users still on the latter. For now, we're going to revert Go module support because there doesn't seem to be any easy way (i.e., anything that's not maintaining a fork) of doing a good job of both. Projects can still use the Go module system, but will have to pull this package in as a pre-module incompatible one. We are hoping to eventually see basic module awareness merged into Dep: golang/dep#1963 If/when that gets done, we'll revert this revert after a small grace period, and hopefully be back to a place where both packaging systems are well supported. I'll also modify the README to clarify the current situation in another follow up PR.
|
@mdestagnol Dep should be working once again with version 55.10.0. |
|
Awesome, thx @brandur-stripe, quickly checked and dep did update to 55.10! By the way really appreciate how responsive you've been on this issue, merging pull requests, etc.; it really feels like a premium developer experience! Thx a lot |
|
@mdestagnol Excellent! And no worries at all — thanks for sticking through it. |
Similar to the original implementation for Go Modules in #712, here we add a `go.mod` and `go.sum`, then proceed to use Go Modules style import paths everywhere that include the current major revision. Unfortunately, this may still cause trouble for Dep users who are trying to upgrade stripe-go, but the project's now had two years to help its users with basic Go Module awareness, and has chosen not to do so. It's received no commits of any kind since August 2019, and therefore would be considered unmaintained by most definitions. Elsewhere, Go Modules now seem to be the only and obvious way forward, so we're likely to see more and more users on them. `scripts/check_api_clients/main.go` is also updated to be smarter about breaking down package paths which may now include the major. [1] golang/dep#1963
Similar to the original implementation for Go Modules in #712, here we add a `go.mod` and `go.sum`, then proceed to use Go Modules style import paths everywhere that include the current major revision. Unfortunately, this may still cause trouble for Dep users who are trying to upgrade stripe-go, but the project's now had two years to help its users with basic Go Module awareness, and has chosen not to do so. It's received no commits of any kind since August 2019, and therefore would be considered unmaintained by most definitions. Elsewhere, Go Modules now seem to be the only and obvious way forward, so we're likely to see more and more users on them. `scripts/check_api_clients/main.go` is also updated to be smarter about breaking down package paths which may now include the major. [1] golang/dep#1963
Similar to the original implementation for Go Modules in #712, here we add a `go.mod` and `go.sum`, then proceed to use Go Modules style import paths everywhere that include the current major revision. Unfortunately, this may still cause trouble for Dep users who are trying to upgrade stripe-go, but the project's now had two years to help its users with basic Go Module awareness, and has chosen not to do so. It's received no commits of any kind since August 2019, and therefore would be considered unmaintained by most definitions. Elsewhere, Go Modules now seem to be the only and obvious way forward, so we're likely to see more and more users on them. `scripts/check_api_clients/main.go` is also updated to be smarter about breaking down package paths which may now include the major. [1] golang/dep#1963
* Make API response accessible on returned API structs (#1054) * Make API response accessible on returned API structs Makes an API response struct containing niceties like the raw response body, status, and request ID accessible via API resource structs returned from client functions. For example: customer, err := customer.New(params) fmt.Printf("request ID = %s\n", customer.LastResponse.RequestID) This is a feature that already exists in other language API libraries and which is requested occasionally here, usually for various situations involving more complex usage or desire for better observability. -- Implementation We introduce a few new types to make this work: * `APIResponse`: Represents a response from the Stripe API and includes things like request ID, status, and headers. I elected to create my own object instead of reusing `http.Response` because it gives us a little more flexibility, and hides many of myriad of fields exposed by the `http` version, which will hopefully give us a little more API stability/forward compatibility. * `APIResource`: A struct that contains `LastResponse` and is meant to represent any type that can we returned from a Stripe API endpoint. A coupon is an `APIResource` and so is a list object. This struct is embedded in response structs where appropriate across the whole API surface area (e.g. `Coupon`, `ListMeta`, etc.). * `LastResponseGetter`: A very basic interface to an object that looks like an `APIResource`. This isn't strictly necessary, but gives us slightly more flexibility around the API and makes backward compatibility a little bit better for non-standard use cases (see the section on that below). `stripe.Do` and other backend calls all start taking objects which are `LastResponseGetter` instead of `interface{}`. This provides us with some type safety around forgetting to include an embedded `APIResource` on structs that should have it by making the compiler balk. As `stripe.Do` finishes running a request, it generates an `APIResponse` object and sets it onto the API resource type it's deserializing and returning (e.g. a `Coupon`). Errors also embed `APIResource` and similarly get access to the same set of fields as response resources, although in their case some of the fields provided in `APIResponse` are duplicates of what they had already (see "Caveats" below). -- Backwards compatibility This is a minor breaking change in that backend implementations methods like `Do` now take `LastResponseGetter` instead of `interface{}`, which is more strict. The good news though is that: * Very few users should be using any of these even if they're technically public. The resource-specific clients packages tend to do all the work. * Users who are broken should have a very easy time updating code. Mostly this will just involve adding `APIResource` to structs that were being passed in. -- Naming * `APIResponse`: Went with this instead of `StripeResponse` as we see in some other libraries because the linter will complain that it "stutters" when used outside of the package (meaning, uses the same word twice in a row), i.e. `stripe.StripeResponse`. `APIResponse` sorts nicely with `APIResource` though, so I think it's okay. * `LastResponse`: Copied the "last" convention from other API libraries like stripe-python. * `LastResponseGetter`: Given an "-er" name per Go convention around small interfaces that are basically one liners -- e.g. `Reader`, `Writer, `Formatter`, `CloseNotifier`, etc. I can see the argument that this maybe should just be `APIResourceInterface` or something like that in case we start adding new things, but I figure at that point we can either rename it, or create a parent interface that encapsulates it: ``` go type APIResourceInterface interface { LastResponseGetter } ``` -- Caveats * We only set the last response for top-level returned objects. For example, an `InvoiceItem` is an API resource, but if it's returned under an `Invoice`, only `Invoice` has a non-nil `LastResponse`. The same applies for all resources under list objects. I figure that doing it this way is more performant and makes a little bit more intuitive sense. Users should be able to work around it if they need to. * There is some duplication between `LastResponse` and some other fields that already existed on `stripe.Error` because the latter was already exposing some of this information, e.g. `RequestID`. I figure this is okay: it's nice that `stripe.Error` is a `LastResponseGetter` for consistency with other API resources. The duplication is a little unfortunate, but not that big of a deal. * Rename `LastResponseGetter` to `LastResponseSetter` and remove a function * Update stripe.go Co-Authored-By: Olivier Bellone <[email protected]> * Move `APIResource` onto individual list structs instead of having it in `ListMeta` Co-authored-by: Brandur <[email protected]> Co-authored-by: Olivier Bellone <[email protected]> * Remove all beta features from Issuing APIs * Multiple breaking API changes * `PaymentIntent` is now expandable on `Charge` * `Percentage` was removed as a filter when listing `TaxRate` * Removed `RenewalInterval` on `SubscriptionSchedule` * Removed `Country` and `RoutingNumber` from `ChargePaymentMethodDetailsAcssDebit` * Start using Go Modules Similar to the original implementation for Go Modules in #712, here we add a `go.mod` and `go.sum`, then proceed to use Go Modules style import paths everywhere that include the current major revision. Unfortunately, this may still cause trouble for Dep users who are trying to upgrade stripe-go, but the project's now had two years to help its users with basic Go Module awareness, and has chosen not to do so. It's received no commits of any kind since August 2019, and therefore would be considered unmaintained by most definitions. Elsewhere, Go Modules now seem to be the only and obvious way forward, so we're likely to see more and more users on them. `scripts/check_api_clients/main.go` is also updated to be smarter about breaking down package paths which may now include the major. [1] golang/dep#1963 * Change v71 back to v70 Move back down to current major version so that we can test that our release script will bump it to v71 properly when the time comes. Co-authored-by: Brandur <[email protected]> Co-authored-by: Olivier Bellone <[email protected]> Co-authored-by: Remi Jannel <[email protected]>
Go modules takes the advise that you should change the import path on all breaking changes to a package and formalizes it as the import compatibility rule.
As annoying as it is to update the major version in multiple places (tags, imports, the mod file), Go modules doesn't really work right unless you do. For example, right now if I add the following to my
go.modfile:after I build it will be updated to something like the following because it doesn't think the version is correct:
This change fixes the module name to include the
v52suffix and updates all imports so that an older version of the library doesn't get included in the v52 version.There are other ways to manage this aside from bumping the module version, eg. copying the entire package into a
v52/tree, or maintaining an internal base package and only overriding new things in avXXtree, branches, etc. but those all seemed like even more of a pain to me.This is probably a pretty big decision, so this PR is more about getting discussion started about supporting Go modules more fully than anything else.
It should be noted that old supported Go versions have been updated in a point release to understand this so it shouldn't break anything on other releases to change the module name.
For more information on preparing a release see https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher