Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

Fixes for docker 17.06.0 #825

Merged
merged 12 commits into from
Jul 12, 2017
Merged

Fixes for docker 17.06.0 #825

merged 12 commits into from
Jul 12, 2017

Conversation

johnflavin
Copy link
Contributor

@johnflavin johnflavin commented Jul 11, 2017

In fixing #822, I saw that a lot of tests were failing on docker 17.06.0. I fixed all the errors in both tests and code so that the tests run cleanly on my machine.

However, we can't test 17.06.0 on travis yet because it isn't in the docker apt repo.

@codecov-io
Copy link

codecov-io commented Jul 11, 2017

Codecov Report

Merging #825 into master will increase coverage by 0.17%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master     #825      +/-   ##
============================================
+ Coverage     66.21%   66.38%   +0.17%     
- Complexity      737      738       +1     
============================================
  Files           165      165              
  Lines          3123     3130       +7     
  Branches        357      357              
============================================
+ Hits           2068     2078      +10     
+ Misses          897      893       -4     
- Partials        158      159       +1

@olegtikhonov
Copy link

Is there a way to get the "fixed" jar somehow? with tag .... "alpha/beta"?

Copy link
Member

@mattnworb mattnworb left a comment

Choose a reason for hiding this comment

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

LGTM, although I don't know anything about new Docker Remote API changes.

Could you just add something to the changelog too?

@@ -5099,13 +5117,22 @@ public void testCreateServiceWithSecretHostnameHostsAndHealthcheck() throws Exce
.build();

final String[] commandLine = {"ping", "-c4", "localhost"};
final long interval = dockerApiVersionLessThan("1.30") ? 30L : 30000000L;
Copy link
Member

Choose a reason for hiding this comment

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

did they change the unit in between api versions? 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they did, yes. Or maybe they were lenient about the units before, and started being more strict. I think these were always intended to be in nanoseconds (at least our javadoc says they are in nanoseconds).

@johnflavin
Copy link
Contributor Author

I can add something to the change log, but I want to be careful about the wording. I didn't actually bring us up to full compatibility with the new API. I mostly just made sure the tests we run are asserting accurate stuff, and in a couple places I made small modifications to the code where the tests exposed things that were broken.

@mattnworb
Copy link
Member

@johnflavin sounds good to me, I think any extra info in the changelog is helpful and better than none. Thanks!

Copy link
Member

@mattnworb mattnworb left a comment

Choose a reason for hiding this comment

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

thanks!

@mattnworb mattnworb merged commit dddfb26 into spotify:master Jul 12, 2017
@johnflavin johnflavin deleted the 17.06 branch July 12, 2017 19:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants