Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix #682 - Add LogPath to docker inspect response#683

Merged
KostyaSha merged 4 commits into
docker-java:masterdocker-java/docker-java:masterfrom
jkubrynski:682-logpath-inspectjkubrynski/docker-java:682-logpath-inspectCopy head branch name to clipboard
Aug 28, 2016
Merged

Fix #682 - Add LogPath to docker inspect response#683
KostyaSha merged 4 commits into
docker-java:masterdocker-java/docker-java:masterfrom
jkubrynski:682-logpath-inspectjkubrynski/docker-java:682-logpath-inspectCopy head branch name to clipboard

Conversation

@jkubrynski

@jkubrynski jkubrynski commented Aug 27, 2016

Copy link
Copy Markdown
Contributor

This change is Reviewable

@KostyaSha

Copy link
Copy Markdown
Member

Could you create some test case?

private String hostsPath;

@JsonProperty("LogPath")
private String logPath;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since what api version?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's present since I remember. Even in your test inspectContainerResponse_full.json there is LogPath element

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please try check whether it exist before 1.19 at least and place javadoc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it already exists in existing json samples, then extend deserialisation test case with check.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LogPath was for sure in 1.18

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please place javadoc @since ~ >1.17

@KostyaSha

Copy link
Copy Markdown
Member

assertEquals(response.getVolumesRW()[1].getVolume().getPath(), "/bar/foo/myvol2");
assertFalse(response.getVolumesRW()[1].getAccessMode().toBoolean());
assertTrue(response.getVolumesRW()[0].getAccessMode().toBoolean());
assertEquals(response.getLogPath(), "/mnt/sda1/var/lib/docker/containers/469e5edd8d5b33e3c905a7ffc97360ec6ee211d6782815fbcd144568045819e1/469e5edd8d5b33e3c905a7ffc97360ec6ee211d6782815fbcd144568045819e1-json.log");

@KostyaSha KostyaSha Aug 27, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Bad example was before...
assertThat(response.getLogPath(), is("/mnt/sda1/var/lib/docker/containers/469e5edd8d5b33e3c905a7ffc97360ec6ee211d6782815fbcd144568045819e1/469e5edd8d5b33e3c905a7ffc97360ec6ee211d6782815fbcd144568045819e1-json.log")

@KostyaSha KostyaSha added this to the 3.0.6 milestone Aug 27, 2016
@KostyaSha KostyaSha closed this Aug 28, 2016
@KostyaSha KostyaSha reopened this Aug 28, 2016
@KostyaSha

Copy link
Copy Markdown
Member

Retriggering PR build

@KostyaSha KostyaSha closed this Aug 28, 2016
@KostyaSha KostyaSha reopened this Aug 28, 2016
@KostyaSha KostyaSha merged commit f44616f into docker-java:master Aug 28, 2016
panuse pushed a commit to TuKangTech/docker-java that referenced this pull request Aug 20, 2017
…java#683)

* Fix docker-java#682 - Add LogPath to docker inspect response

* Fix docker-java#682 - Add LogPath to docker inspect response - fixes after code review

* Fix docker-java#682 - Add LogPath to docker inspect response - add javadoc

* Fix docker-java#682 - Add LogPath to docker inspect response - clean test assertion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.