Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3194)

Issue 28190043: Check host supports containers when adding (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by wallyworld
Modified:
10 years, 4 months ago
Reviewers:
mp+195543, thumper
Visibility:
Public.

Description

Check host supports containers when adding When add-machine is used to add a container to a machine, a check is done to see if the host machine supports the requested container type. If this information is known, and the container type is not supported, an error is returned. If the information is not yet known, it assumes it will be ok. A followup branch will perform a check before the container is to be provisioned and any error will be recorded then. https://code.launchpad.net/~wallyworld/juju-core/add-machine-supported-containers/+merge/195543 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 1

Patch Set 2 : Check host supports containers when adding #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -9 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/addmachine_test.go View 1 chunk +10 lines, -0 lines 0 comments Download
M state/apiserver/client/client_test.go View 1 1 chunk +33 lines, -8 lines 1 comment Download
M state/state.go View 1 chunk +17 lines, -1 line 0 comments Download
M state/state_test.go View 1 chunk +55 lines, -0 lines 0 comments Download

Messages

Total messages: 4
wallyworld
Please take a look.
10 years, 4 months ago (2013-11-18 04:42:13 UTC) #1
thumper
https://codereview.appspot.com/28190043/diff/1/state/apiserver/client/client_test.go File state/apiserver/client/client_test.go (right): https://codereview.appspot.com/28190043/diff/1/state/apiserver/client/client_test.go#newcode1539 state/apiserver/client/client_test.go:1539: // Create a machine to host a requested container. ...
10 years, 4 months ago (2013-11-19 02:26:47 UTC) #2
wallyworld
Please take a look.
10 years, 4 months ago (2013-11-19 03:22:24 UTC) #3
thumper
10 years, 4 months ago (2013-11-19 03:59:15 UTC) #4
LGTM with removing the switch.

https://codereview.appspot.com/28190043/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/28190043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1570: for i, machineResult := range
machines {
I think it would be much clearer if you did the explicit check for each of the
machineResults rather than use the switch.

It looks like 0 and 1 are identical, why test the same thing twice?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b