Skip to content

fixes #7802, when api version 1.11 is json.Marshaling the container st...#7914

Merged
tiborvass merged 1 commit intomoby:masterfrom
jessfraz:7802-backwards-compat-state
Sep 8, 2014
Merged

fixes #7802, when api version 1.11 is json.Marshaling the container st...#7914
tiborvass merged 1 commit intomoby:masterfrom
jessfraz:7802-backwards-compat-state

Conversation

@jessfraz
Copy link
Copy Markdown
Contributor

@jessfraz jessfraz commented Sep 5, 2014

fixes #7802, when api version 1.11 is json.Marshaling the container struct

Signed-off-by: Jessica Frazelle [email protected]

@tiborvass
Copy link
Copy Markdown
Contributor

tiborvass commented Sep 5, 2014

I think I would prefer reverting the State embedding instead of adding json struct tags.
@LK4D4 is there a particular reason you embedded State in Container in #7775 ?

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Sep 6, 2014

Yes, I did this for merging mutexes :)
Seems like we need some test for detecting such things.

@jessfraz
Copy link
Copy Markdown
Contributor Author

jessfraz commented Sep 6, 2014

Where should we start putting API tests?
On Sep 6, 2014 12:25 AM, "Alexandr Morozov" [email protected]
wrote:

Yes, I did this for merging mutexes :)
Seems like we need some test for detecting such things.

—
Reply to this email directly or view it on GitHub
#7914 (comment).

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Sep 6, 2014

ping @tiborvass @unclejack

@jessfraz
Copy link
Copy Markdown
Contributor Author

jessfraz commented Sep 6, 2014

Also applies to the comment in #7886
On Sep 6, 2014 10:05 AM, "Alexandr Morozov" [email protected]
wrote:

ping @tiborvass https://github.com/tiborvass @unclejack
https://github.com/unclejack

—
Reply to this email directly or view it on GitHub
#7914 (comment).

@jessfraz jessfraz force-pushed the 7802-backwards-compat-state branch from 8f6f056 to 66568b7 Compare September 7, 2014 21:48
@jessfraz
Copy link
Copy Markdown
Contributor Author

jessfraz commented Sep 7, 2014

ping @LK4D4 @tiborvass @unclejack

Added a test, also this PR and #7886 add the sockRequest function to test-integration-cli utils, so depending which get merged first I'll rebase and fix the other.

LK4D4
LK4D4 reviewed Sep 8, 2014
View reviewed changes
Comment thread integration-cli/docker_utils.go Outdated
Copy link
Copy Markdown
Contributor

@LK4D4 LK4D4 Sep 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pretty confusing to have named returns in this case, because I always need to look what the hell is body now. I prefer to return just nil instead of body on errors.

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Sep 8, 2014

Can we write here test that check not only state? Because I think next time error will be not in state.

tiborvass
tiborvass reviewed Sep 8, 2014
View reviewed changes
Comment thread daemon/container.go Outdated
Copy link
Copy Markdown
Contributor

@tiborvass tiborvass Sep 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a same-line comment to say that this is needed for the remote api version <= 1.11

@jessfraz jessfraz force-pushed the 7802-backwards-compat-state branch from 66568b7 to 98a67a0 Compare September 8, 2014 16:01
@jessfraz
Copy link
Copy Markdown
Contributor Author

jessfraz commented Sep 8, 2014

updated ping @LK4D4 @tiborvass

@jessfraz jessfraz force-pushed the 7802-backwards-compat-state branch from 98a67a0 to 42737d1 Compare September 8, 2014 16:08
LK4D4
LK4D4 reviewed Sep 8, 2014
View reviewed changes
Comment thread daemon/container.go
Copy link
Copy Markdown
Contributor

@LK4D4 LK4D4 Sep 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, isn't this needed for all api versions?

Copy link
Copy Markdown
Contributor Author

@jessfraz jessfraz Sep 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because new versions use .Set("State", container.State) versus just json.Marshal-ing the struct

Copy link
Copy Markdown
Contributor

@tiborvass tiborvass Sep 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Starting from 1.12 we set manually all the fields: https://github.com/docker/docker/blob/master/daemon/inspect.go#L19 (raw is set only for <= 1.11)

@jessfraz jessfraz force-pushed the 7802-backwards-compat-state branch from 42737d1 to 948f6d4 Compare September 8, 2014 16:25
@jessfraz
fixes moby#7802, when api version 1.11 is json.Marshaling the conta…
f49c3f2 
…iner struct

Signed-off-by: Jessica Frazelle <[email protected]>

Docker-DCO-1.1-Signed-off-by: Jessica Frazelle <[email protected]> (github: )
@jessfraz jessfraz force-pushed the 7802-backwards-compat-state branch from 948f6d4 to f49c3f2 Compare September 8, 2014 16:30
@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Sep 8, 2014

Cool, LGTM

@tiborvass
Copy link
Copy Markdown
Contributor

tiborvass commented Sep 8, 2014

LGTM

tiborvass added a commit that referenced this pull request Sep 8, 2014
@tiborvass
Merge pull request #7914 from jfrazelle/7802-backwards-compat-state
68e07f3 
fixes #7802, when api version 1.11 is `json.Marshal`ing the container st...
@tiborvass tiborvass merged commit 68e07f3 into moby:master Sep 8, 2014
@jessfraz jessfraz deleted the 7802-backwards-compat-state branch October 10, 2014 03:15
@thaJeztah thaJeztah mentioned this pull request Oct 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

State field missing in remote api <=1.11

3 participants

@jessfraz @tiborvass @LK4D4