Skip to content

fix for issue 507#516

Closed
Hendrik-H wants to merge 2 commits intodocker-java:masterfrom
Hendrik-H:master
Closed

fix for issue 507#516
Hendrik-H wants to merge 2 commits intodocker-java:masterfrom
Hendrik-H:master

Conversation

@Hendrik-H
Copy link
Copy Markdown
Contributor

@Hendrik-H Hendrik-H commented Mar 22, 2016

This change is Reviewable

@Hendrik-H
Copy link
Copy Markdown
Contributor Author

Hendrik-H commented Mar 22, 2016

This should fix issue #507. A new test is also provided. It is basically using the data that failed.

@Hendrik-H Hendrik-H closed this Mar 22, 2016
KostyaSha
KostyaSha reviewed Mar 22, 2016
View reviewed changes
src/test/resources/samples/1.22/info/2.json
@@ -0,0 +1,165 @@
{
Copy link
Copy Markdown
Member

@KostyaSha KostyaSha Mar 22, 2016

Choose a reason for hiding this comment

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

is it real dump or copy-paste of existed?

Copy link
Copy Markdown
Contributor Author

@Hendrik-H Hendrik-H Mar 22, 2016

Choose a reason for hiding this comment

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

You might notice that the DriverStatus part is quite different. Took me some time to put that all in. Also the other IDs, OS and so on are different. This is a dump from my machine. the only thing changed is the host name.

Copy link
Copy Markdown
Member

@KostyaSha KostyaSha Mar 22, 2016

Choose a reason for hiding this comment

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

👍 Hard work, i spent a lot of time filling and comparing all this things initially.

@Hendrik-H Hendrik-H reopened this Mar 22, 2016
@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 22, 2016

Current coverage is 22.78%

Merging #516 into master will decrease coverage by -0.01% as of 0560f03

@@            master    #516   diff @@
======================================
  Files          294     294       
  Stmts         6077    6078     +1
  Branches       526     526       
  Methods          0       0       
======================================
  Hit           1385    1385       
  Partial         83      83       
- Missed        4609    4610     +1

Review entire Coverage Diff as of 0560f03

Powered by Codecov. Updated on successful CI builds.

@Hendrik-H
Copy link
Copy Markdown
Contributor Author

Hendrik-H commented Mar 22, 2016

sorry, have no test for this but the original is very obviously wrong.

KostyaSha
KostyaSha reviewed Mar 22, 2016
View reviewed changes
src/test/java/com/github/dockerjava/api/model/InfoTest.java
final JavaType type = mapper.getTypeFactory().constructType(Info.class);

final Info info = testRoundTrip(VERSION_1_22,
"info/2.json",
Copy link
Copy Markdown
Member

@KostyaSha KostyaSha Mar 22, 2016

Choose a reason for hiding this comment

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

Could you rename filename to something meaningful to initial issue that you had?
I called initial dump as 1.json because it was the first real dump, but it seems was bad idea to name file in such way.

Copy link
Copy Markdown
Member

@KostyaSha KostyaSha Mar 22, 2016

Choose a reason for hiding this comment

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

or if it tests everything, then you can keep it.

@KostyaSha
Copy link
Copy Markdown
Member

KostyaSha commented Mar 22, 2016

Also if it possible please PR single issues into single PRs as we are placing issue into milestones and it would be easier to list milestone issues when they 1 issue per 1 PR. (not critical but feel free to create as more prs as you need). Thanks.

@Hendrik-H
Copy link
Copy Markdown
Contributor Author

Hendrik-H commented Mar 22, 2016

This was my first PR, so not really certain how to do this. I did one commit per change but the second PR showed all the changes from the first as well.

@KostyaSha
Copy link
Copy Markdown
Member

KostyaSha commented Mar 22, 2016

Usually i do the next:

  1. git fetch --all
  2. git checkout upstream/master (where upstream this repo)
  3. git checkout -b issueXXX
  4. make changes
  5. git push (it will provide command for pushing to origin) (where origin is your forked repo)
  6. open PR from your fork (gh will suggest it automatically when you navigate to upstream repo)

then checkout master back
6) git checkout master
7) git checkout -b issueYYY
.. all the same commands

That will allow you jumping between branches.
It case of amend/fixup/rebase you should do force push.

@KostyaSha
Copy link
Copy Markdown
Member

KostyaSha commented Mar 22, 2016

Could you add test for paths and i will handle commits to do the merge myself then?

@Hendrik-H
Copy link
Copy Markdown
Contributor Author

Hendrik-H commented Mar 22, 2016

I'll try to write a testcase.

@KostyaSha KostyaSha self-assigned this Mar 23, 2016
@KostyaSha KostyaSha mentioned this pull request Mar 27, 2016
Merged
@KostyaSha KostyaSha added this to the 3.0.0-RC4 milestone Mar 27, 2016
@KostyaSha
Copy link
Copy Markdown
Member

KostyaSha commented Mar 27, 2016

I picked your commit and merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@KostyaSha KostyaSha

Labels

type/bug

Projects

None yet

Milestone

3.0.0-RC4

Development

Successfully merging this pull request may close these issues.

3 participants

@Hendrik-H @codecov-io @KostyaSha