search

ncw / swift

Go language interface to Swift / Openstack Object Storage / Rackspace cloud files (golang)
MIT License
310 stars 107 forks source link

Allow context propagation #162

Closed FUSAKLA closed 3 years ago

FUSAKLA commented 3 years ago

Resolves #159 #161

Hi, as suggested in the issue, I'm adding the *WithContext functions for all the already existing where it makes sense to pass down the context. Former functions are preserved and calls the new ones using the context.Background() to avoid breaking the API of library.

Only breaking change is the change in the Authenticator interface where from now on the context is required. I could add a new function RequestWithContext to the interface but that would still break the API if someone is using the Authenticator interface. Hopefully it's ok this way.

Tests are passing with original functions used which are now using the WithContext in background so hopefully should all work :crossed_fingers:

FUSAKLA commented 3 years ago

Hm, I see the NewRequestWithContext was added in Go 1.13 ... :/ We will probably need to use build tags for this right? Something like // +build go1.13? @ncw

ncw commented 3 years ago

Hi, as suggested in the issue, I'm adding the *WithContext functions for all the already existing where it makes sense to pass down the context. Former functions are preserved and calls the new ones using the context.Background() to avoid breaking the API of library.

OK

Only breaking change is the change in the Authenticator interface where from now on the context is required. I could add a new function RequestWithContext to the interface but that would still break the API if someone is using the Authenticator interface. Hopefully it's ok this way.

Gah. That is annoying!

So we need to add ctx to the interface methods (we should add it to all of them even if the current implementation doesn't use them). but we can't do so without breaking backwards compatibility.

That is making me reconsider the plan - maybe we should be bumping the major version.

Tests are passing with original functions used which are now using the WithContext in background so hopefully should all work

Good.

I see the NewRequestWithContext was added in Go 1.13 ... :/ We will probably need to use build tags for this right? Something like // +build go1.13?

Yes build tags will be necessary.

This is another reason for bumping the major version I think - currently we support everything back to go1! Dropping support for old go versions will enable us to lose quite a bit of code too.

I was hoping to avoid bumping the major version to avoid the whole v2 problems, but I don't think we can.

My preferred approach would be not to create a v2 subdirectory but to rename the module in the go.mod github.com/ncw/swift/v2. Users will then have to take a deliberate action to upgrade to v2.

So what do you think about changing this PR to add ctx to all the original methods and also to all the methods in Authenticator? I can then merge it to a v2 branch and we can work on the other things that it would be nice to have in a v2 release.

FUSAKLA commented 3 years ago

Hi, sorry for the lag. Yes the breaking change is unfortunate. Ok that seems fair. TBH I have never dealt with a multiple simultaneous major versions, so I'll leave this to you to decide.

SO I'm closing this one and opening a new one with the ctx added and switchung to v2

FUSAKLA commented 3 years ago

replaced with https://github.com/ncw/swift/pull/164