Skip to content

Conversation

calavera
Copy link
Contributor

@calavera calavera commented Oct 23, 2015

@calavera
Copy link
Contributor Author

I added this to the milestone because it's a bug we introduced in master recently.

@cpuguy83
Copy link
Member

I don't think this bug was in 1.9.0 unless we do indeed want to check for json in 1.9.

@runcom
Copy link
Member

runcom commented Oct 23, 2015

I remember the pr which introduced this check

@calavera
Copy link
Contributor Author

@cpuguy83 you're right, this is not in 1.9.

@calavera calavera removed this from the 1.9.0 milestone Oct 23, 2015
@thaJeztah
Copy link
Member

LGTM, thanks @calavera

@cpuguy83
Copy link
Member

Need to bump the API version in the version check.

@thaJeztah
Copy link
Member

oh cr*p you're right #etoomanyversions

@calavera calavera force-pushed the version_exec_json_check branch from 802850e to 9d97fdf Compare October 23, 2015 21:52
@calavera
Copy link
Contributor Author

fixed, thanks!

@cpuguy83
Copy link
Member

Sorry, one more thing... Test for pre 1.22 behavior?
Also seems like there probably isn't any test ensuring json is The only content-type supported here going forward either.

Keep backwards compatibility with old versions of the api.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the version_exec_json_check branch from 9d97fdf to 0b5e628 Compare October 26, 2015 19:12
@calavera
Copy link
Contributor Author

I added a test to check that we keep backwards compatibility.

Also seems like there probably isn't any test ensuring json

@cpuguy83 it looks like we had already a test sending "application/json" content types to the start endpoint. It didn't fail before because the didn't check the content type, and it didn't fail when we forced it with CheckJSON because it was the expected content type.

All in all, this ensures we keep backwards compatibility and adds a test to verify it. We already have a test that checks the current valid behavior.

@cpuguy83
Copy link
Member

LGTM

1 similar comment
@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request Oct 26, 2015
@crosbymichael crosbymichael merged commit 5cf028d into moby:master Oct 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using docker client 1.8.3 to run exec against a docker daemon built from master fails
7 participants