FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

  1. docker-java
  2. docker-java

Repository navigation

fix for issue 507 - #516

Closed
Hendrik-H wants to merge 2 commits into
docker-java:masterfrom
Hendrik-H:master
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

Hendrik-H commented Mar 22, 2016

Copy link
Copy Markdown
Contributor

This change is

Hendrik-H commented Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

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

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

KostyaSha Mar 22, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

is it real dump or copy-paste of existed?

Hendrik-H Mar 22, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

KostyaSha Mar 22, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Hendrik-H reopened this Mar 22, 2016

codecov-io commented Mar 22, 2016

Copy link
Copy Markdown

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 commented Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

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

KostyaSha reviewed Mar 22, 2016
View reviewed changes
Comment thread 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",

KostyaSha Mar 22, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

KostyaSha Mar 22, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

KostyaSha commented Mar 22, 2016

Copy link
Copy Markdown
Member

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 commented Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

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 commented Mar 22, 2016

Copy link
Copy Markdown
Member

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 commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

Hendrik-H commented Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

I'll try to write a testcase.

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

KostyaSha commented Mar 27, 2016

Copy link
Copy Markdown
Member

I picked your commit and merged to master.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

KostyaSha

Labels

type/bug

Projects

None yet

Milestone

3.0.0-RC4

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL