add context.Context to all calls

[aybabtme]

This adds context.Context to all the API calls. This allows us to insert the ctx in the http.Request.Context(), so that calls can be traced and cancelled and so on.

This is a breaking change.

[mauricio]

LGTM on 💚

[xmudrii]

Doesn't this require rebase and update as many things has been changed since then? Other than that, it looks great! 👍 💯

[aybabtme]

@xmudrii rebase and update was done

[xmudrii]

‎Oh, great. =)I was on mobile and didn't notice it, sorry. 👍

[codenrhoden]

So does mean that you no longer support anyone building with golang < 1.7 ?

I have a project that uses godo, and builds with golang 1.6, 1.7, 1.8, and tip. We have other projects that use our project that are locked to golang 1.6 for now. Updating my godo dep to this version breaks our builds on 1.6.

[codenrhoden]

BTW, I'm mostly just curious if this was a conscious choice. I'm not stuck -- I just won't include the Digital Ocean driver when we build using 1.6. We didn't want to keep building with 1.6 either, but we had downstream users that do.

[codenrhoden]

My colleague @akutz submitted #137, which introduces a portable context that works with versions of go prior to 1.7.

[aybabtme]

@codenrhoden 1.6 wasn't taken in consideration, no. It's more because we need the context propagation ourselves. I didn't quite think of 1.6 lacking support for it when I submitted the PR.

However, my personal point of view on Go 1.6 however is that it's a >2 release-old version and I think most projects supporting 2 versions behind is a fair tradeoff; hence that dropping support for 1.6 is ok. Many other packages follow this convention in the ecosystem. I'm not the biggest fan of back-porting context.Context for that matter, it feels to me like a high support overhead for an ecosystem that's used to vendoring and upgrading language versions rapidly. Again this is just my personal opinion. :)

That said, I'm not on the team that owns this package, so I'll leave it to those who do to make the call.

[xmudrii]

@codenrhoden‎ @aybabtme 1.6 was released year ago - it's not that old release.    However, I think drawbacks for supporting 1.6 at this point are not worth it. Personally I'm very against backporting code just to make something work on older versions.   IMO, it bit hurts project structure, readabi‎lity and makes project much harder to maintain over time.   ‎ 2 release "rule" seems pretty reasonable to me. In that case, developer have at least half year to migrate to newer version, in case something is bit changed. In some cases migrating would not be required but it's recommended. Beside so, I don't think that migrating in case of Go is very complicated but that depends on project.   ‎ This was my 2 cents. Note if needed - I'm just contributor here, not affiliated with project in any other way. We will see what project maintainers have to say.‎

[akutz]

I wrote the patch to which @codenrhoden refers. I suggest everyone take a look. It's quite simple in nature, in large part to the existing organization of this package.

That said, I also argued to @codenrhoden that libStorage and REX-Ray adheres to the previous two versions of Go mentality. However, at the same time what made me reintroduce support for Go 1.6 earlier this year are the following reasons:

1. People are using it, such as Mesos and DC/OS.
2. 1.6 isn't that old, and doesn't really fall under the two-previous versions rule if you consider 1.8 did not release until earlier this year. In any other situation I don't think we'd consider EOLing support in favor of a brand-new release. For example, I did not move REX-Ray to 1.8 yet as it's still pretty damn new.

As I said, please take a look at the patch. I simply centralized the Context-specific code into a package that uses build tags and copied the Context interface locally. It's what I do for libStorage as well as other platform packages such as GoIsilon, GoScaleIO, etc. I know plenty of other packages do this as well. It's one of the beautiful benefits of Go interfaces :)

[akutz]

Hi all,

I also just noticed that this project's .travis.yml file is building against Go 1.7, not Go 1.7.5. Unlike Docker versions of Go where 1.X always refers to 1.X.Y, Travis requires explicit versions. My PR's build shows:

$ go version
go version go1.7 linux/amd64

I've updated my PR to also build against Go 1.6.3 (with an allowed failure). I suggest updating the build to explicitly specify Go 1.7.5 as well.

[akutz]

Hi all,

One last thing. The PR just successfully built and tested with Go 1.6.3 :)

[akutz]

Hi @xmudrii,

I agree that 1.6 isn't that old. It was released a year ago last month and 1.7 was released last August. To your following concerns:

Personally I'm very against backporting code just to make something work on older versions.

Nothing has been backported.

IMO, it bit hurts project structure, readabi‎lity

Thanks to the flat, simple structure of the godo package, the addition of a godo/context package has not impinged the project's structure or hurt readability in any way that would prevent me from accepting this PR on one of my own projects. And ask @codenrhoden, I'm a real jerk. I don't like anyone or any code :)

Most of all, this PR was labelled a breaking change due to the use of req.WithContext. Well, it's only a breaking change because of the way this PR elected to implement the intended result. Using a simple build tag-driven solution the same result is achieved in #137 that negates the breaking change.

Thank you!