Skip to content
This repository has been archived by the owner on Nov 23, 2019. It is now read-only.

Remove /json from API URLs #255

Merged
merged 1 commit into from
Jun 7, 2016

Conversation

bfirsh
Copy link
Contributor

@bfirsh bfirsh commented Jun 3, 2016

Signed-off-by: Ben Firshman <[email protected]>
@calavera
Copy link
Contributor

calavera commented Jun 6, 2016

LGTM

@bfirsh
Copy link
Contributor Author

bfirsh commented Jun 6, 2016

FWIW this has a hard dependency on moby/moby#23219, and that has stalled on a design decision.

@vdemeester
Copy link
Contributor

Moving to code-review as moby/moby#23219 is in code-review

@vdemeester
Copy link
Contributor

LGTM 🐹

@calavera
Copy link
Contributor

calavera commented Jun 7, 2016

still LGTM

@calavera calavera merged commit 739ad16 into docker:master Jun 7, 2016
@tamird
Copy link
Contributor

tamird commented Jun 7, 2016

So...this makes this library not work with older docker versions? For example, the latest version offered by CircleCI is 1.10.

@thaJeztah
Copy link
Member

Oh, yes, I wonder if this should be reverted, given that there's no full agreement yet in moby/moby#23219 (comment)

@vdemeester
Copy link
Contributor

@tamird arf, yeah that is pretty bad 😓
I'll revert this for now and we'll find a better way to handle it

@tamird
Copy link
Contributor

tamird commented Jun 7, 2016

Tim BL had it right all the way back in 1998 https://www.w3.org/Provider/Style/URI.html

@vdemeester
Copy link
Contributor

@tamird the old URL won't change (probably for the next… 5 years or eternity) but the client code might… although we didn't do right… Sorry about that 😓

@tamird
Copy link
Contributor

tamird commented Jun 7, 2016

This client code should also not change until all docker versions that don't support these URLs are EOL'd.

Making this change now is breaking compatibility for no benefit whatsoever, especially given the state of dependency management in Go.

@vdemeester
Copy link
Contributor

@tamird see #263 🙇

@tamird
Copy link
Contributor

tamird commented Jun 7, 2016

👍 thanks

@bfirsh
Copy link
Contributor Author

bfirsh commented Jun 7, 2016

My bad. I should have closed this while there was still a design decision in moby/moby#23219.

I wonder if we should open the discussion about how to version engine-api. People should presumably be locking to API versions somehow rather than using master. (See also #260.)

@bfirsh
Copy link
Contributor Author

bfirsh commented Jun 7, 2016

Ah, nevermind, I see there is cli.version, so I should have used that in this PR to check what URL to make the request to.

Perhaps we should set the default version to the current stable version of Docker so people using engine-api off master don't get broken by default.

@vdemeester
Copy link
Contributor

Perhaps we should set the default version to the current stable version of Docker so people using engine-api off master don't get broken by default.

That's a good idea 👼

@bfirsh
Copy link
Contributor Author

bfirsh commented Jun 7, 2016

@vdemeester I suppose you have to explicitly pass a blank string to client.NewClient if you don't set a version, I guess that's pretty much saying "I expect this to break". I'll add a warning to the godoc that you really should be specifying a version.

It's different for NewEnvClient though: #264

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants