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

Add support for runtime property in HostConfig #1060

Merged
merged 4 commits into from
Aug 29, 2018
Merged

Conversation

vbhavsar
Copy link
Contributor

This PR:

  • Changes the exitCode type from Integer to Long
  • Adds runtime property to HostConfig to support setting runtimes such as nvidia

Vishal Bhavsar added 2 commits August 28, 2018 15:26
This is a continuation of #1053 as parts of docker-client
were still using exit code of Integer. This caused some uses to have inconsistent types.
This commit introduces `runtime` property to HostConfig.
@vbhavsar vbhavsar changed the title Vishal/add runtime Add support for runtime property in HostConfig Aug 28, 2018
Change of exitCode from Integer to Long is a break change.
@mattnworb
Copy link
Member

is the stuff about exit codes related to #1053 ?

also, can you update the CHANGELOG file?

@codecov-io
Copy link

codecov-io commented Aug 28, 2018

Codecov Report

Merging #1060 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1060      +/-   ##
============================================
+ Coverage     67.32%   67.38%   +0.06%     
  Complexity      795      795              
============================================
  Files           178      178              
  Lines          3287     3287              
  Branches        385      385              
============================================
+ Hits           2213     2215       +2     
+ Misses          911      910       -1     
+ Partials        163      162       -1

@mattnworb
Copy link
Member

the change itself LGTM 👍

@caipre
Copy link
Contributor

caipre commented Aug 28, 2018

👍

@vbhavsar
Copy link
Contributor Author

vbhavsar commented Aug 29, 2018

is the stuff about exit codes related to #1053 ?

Yes. The mismatch in exit code was breaking code in helios.

also, can you update the CHANGELOG file?

👍Done.

Copy link
Contributor

@johnflavin johnflavin left a comment

Choose a reason for hiding this comment

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

What you've changed LGTM. But I request one additional change from Integer to Long on ContainerStatus.exitCode.

CHANGELOG.md Outdated
@@ -1,5 +1,13 @@
# Changelog

## 9.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that a bump all the way up to version 9.0.0 is required. 8.13.0 seems more appropriate to me.

CHANGELOG.md Outdated
@@ -1,5 +1,13 @@
# Changelog

## 9.0.0

Released August 29 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to "Not yet released"? The date should be set when the release is made.

@vbhavsar vbhavsar force-pushed the vishal/add-runtime branch from 9ea2db6 to 5a27e80 Compare August 29, 2018 14:10
@vbhavsar
Copy link
Contributor Author

vbhavsar commented Aug 29, 2018

... I request one additional change from Integer to Long on ContainerStatus.exitCode.

@johnflavin That's already part of this PR:
https://github.com/spotify/docker-client/pull/1060/files#diff-47e61bfd4487ca3e3deda5a11d30c091R47

And thanks for the feedback 😃

@vbhavsar vbhavsar merged commit 56da79d into master Aug 29, 2018
@davidxia davidxia deleted the vishal/add-runtime branch October 17, 2018 21:07
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.

5 participants