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

Issue 28790043: Refactor and improve machine supported containers (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years ago by wallyworld
Modified:
5 years, 12 months ago
Reviewers:
mp+195701, jameinel
Visibility:
Public.

Description

Refactor and improve machine supported containers Change the machine supported containers api to set containers rather than adding. In addition, add functionality to iterate any existing containers, and if they are not supported, set their status to error. https://code.launchpad.net/~wallyworld/juju-core/improve-supported-containers-setup/+merge/195701 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+241 lines, -116 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/machine.go View 2 chunks +8 lines, -6 lines 3 comments Download
M state/api/params/internal.go View 1 chunk +5 lines, -5 lines 3 comments Download
M state/api/provisioner/machine.go View 2 chunks +10 lines, -5 lines 0 comments Download
M state/api/provisioner/provisioner_test.go View 2 chunks +15 lines, -2 lines 0 comments Download
M state/apiserver/provisioner/provisioner.go View 2 chunks +8 lines, -4 lines 2 comments Download
M state/apiserver/provisioner/provisioner_test.go View 6 chunks +27 lines, -8 lines 0 comments Download
M state/machine.go View 2 chunks +51 lines, -42 lines 3 comments Download
M state/machine_test.go View 3 chunks +114 lines, -43 lines 2 comments Download
M worker/provisioner/container_initialisation_test.go View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 4
wallyworld
Please take a look.
6 years ago (2013-11-19 03:09:11 UTC) #1
jameinel
I have some comments, and I think there can be a bit more work done. ...
6 years ago (2013-11-20 10:30:10 UTC) #2
wallyworld
https://codereview.appspot.com/28790043/diff/1/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/28790043/diff/1/cmd/jujud/machine.go#newcode235 cmd/jujud/machine.go:235: return fmt.Errorf("clearing supported containers for %s: %v", tag, err) ...
5 years, 12 months ago (2013-11-21 12:35:32 UTC) #3
wallyworld
5 years, 12 months ago (2013-11-23 10:37:06 UTC) #4
https://codereview.appspot.com/28790043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/28790043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:25: type SetSupportedContainersParams struct {
On 2013/11/20 10:30:10, jameinel wrote:
> Given William's comment about "naming this what it is vs what it does"
> 
> Would this be better as just:
> 
> type ContainerParams struct {
>   Machines []MachineContainers
> }
> 
> or something along those lines?
> 
> I can understand that it is the explicit params being passed to a given API,
so
> probably not a strict here, but I also don't think it would be terrible if 2
> APIs shared the parameters, which means maybe it shouldn't be named after the
> api.


I renamed to MachineContainersParams
Sign in to reply to this message.

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