Home Original page

fix for issue 507 by Hendrik-H · Pull Request #516 · docker-java/docker-java

@Hendrik-H

This change is Reviewable

@Hendrik-H

@Hendrik-H

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

KostyaSha

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?

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.

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

@codecov-io

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

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

KostyaSha

final JavaType type = mapper.getTypeFactory().constructType(Info.class);

final Info info = testRoundTrip(VERSION_1_22,
"info/2.json",

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.

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

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

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

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

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

@Hendrik-H

I'll try to write a testcase.

@KostyaSha

I picked your commit and merged to master.