Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Command pool list #14
Conversation
anastasiamac
added some commits
Jan 28, 2015
axw
reviewed
Jan 28, 2015
| +// PoolList lists pools according to a given filter. | ||
| +// If no filter was provided, this will return a list | ||
| +// of all pools. | ||
| +func (c *Client) PoolList(types, names []string) ([]params.PoolInstance, error) { |
axw
Jan 28, 2015
Collaborator
I'd prefer if we stuck with the verb[-object] convention for API method names, and called this ListPools.
axw
reviewed
Jan 28, 2015
| +// If no filter was provided, this will return a list | ||
| +// of all pools. | ||
| +func (c *Client) PoolList(types, names []string) ([]params.PoolInstance, error) { | ||
| + args := params.PoolListFilter{ |
axw
Jan 28, 2015
Collaborator
Maybe call this type "StoragePoolFilter", so it can be reused for other pool-related API calls that need to do filtering.
axw
reviewed
Jan 28, 2015
| + Names: names, | ||
| + Types: types, | ||
| + } | ||
| + found := params.PoolListResults{} |
axw
Jan 28, 2015
Collaborator
"Results" is usually used for bulk method calls, i.e. where there are multiple disparate arguments and corresponding results. As such, this should just be "PoolListResult". I think it should just be "PoolsResult" though.
axw
reviewed
Jan 28, 2015
| +} | ||
| + | ||
| +// PoolListResults holds a collection of pool instances. | ||
| +type PoolListResults struct { |
axw
Jan 28, 2015
Collaborator
I don't think these types make sense. It's not really a bulk call, it's a single call with a single set of filter attributes. I think this ought to be:
type StoragePoolsResult struct {
Pools []StoragePool
}
axw
reviewed
Jan 28, 2015
| @@ -33,3 +33,28 @@ type StorageShowResult struct { | ||
| type StorageListResult struct { | ||
| Instances []StorageInstance | ||
| } | ||
| + | ||
| +// PoolInstance holds data for a pool instance. | ||
| +type PoolInstance struct { |
axw
Jan 28, 2015
Collaborator
s/PoolInstance/StoragePool/
"Pool" isn't very informative without the storage context, and "instance" isn't meaningful here. Pools are always instances.
axw
reviewed
Jan 28, 2015
| + | ||
| +// PoolListFilter holds a filter for pool list API call. | ||
| +type PoolListFilter struct { | ||
| + Names []string |
axw
Jan 28, 2015
Collaborator
Can you comment these please? What are the fields used for? Is there any relationship between Names and Types (i.e. are they pairs?)
anastasiamac
Jan 28, 2015
Added the descriptions for the individual properties. These comments make it obvious that there is no explicit relationship btw Names and Types.
axw
reviewed
Jan 28, 2015
| +type PoolInstance struct { | ||
| + Name string | ||
| + Type string | ||
| + Traits map[string]interface{} |
axw
Jan 28, 2015
Collaborator
I'm not married to the name "config", but I think we should be consistent or we'll have a hard time talking about things as a team. Can you please call it config, and then we can talk about renaming it all to something else?
anastasiamac
Jan 28, 2015
M a bit allergic to config as a var name, especially since we've got more than a few of them. However, I agree that here consistency is important - renamed :D
axw
reviewed
Jan 28, 2015
| +-e, --environment (= "") | ||
| + juju environment to operate in | ||
| +-o, --output (= "") | ||
| + specify an output |
axw
reviewed
Jan 28, 2015
| + c, | ||
| + []string{"--type", "a", "--type", "b", "--name", "xyz", "--name", "abc"}, | ||
| + // Default format is yaml | ||
| + `- name: testName |
axw
Jan 28, 2015
Collaborator
IMO this is much easier to read in the map style. Would you please consider changing it to a map, keyed on the pool name?
axw
reviewed
Jan 28, 2015
| +type PoolInfo struct { | ||
| + Name string `yaml:"name" json:"name"` | ||
| + Type string `yaml:"type" json:"type"` | ||
| + Traits map[string]interface{} `yaml:"characteristics,omitempty" json:"characteristics,omitempty"` |
axw
reviewed
Jan 28, 2015
| + "--name", "xyz", "--name", "abc", | ||
| + "--format", "tabular"}, | ||
| + "TYPE NAME CHARACTERISTICS\n"+ | ||
| + "a testName one=true,two=well,three=maybe\n"+ |
axw
Jan 28, 2015
Collaborator
The k/v map looks inappropriate for tabular. Maybe it's alright. Leave it, and we'll see how it feels in practice.
axw
reviewed
Jan 28, 2015
| @@ -23,6 +23,8 @@ options: | ||
| juju environment to operate in | ||
| -o, --output (= "") | ||
| specify an output file | ||
| +--format (= yaml) | ||
| + specify output format (json|yaml) |
axw
Jan 28, 2015
Collaborator
hold on... why are we even doing this? the cmd code should list flags for us?
anastasiamac
Jan 28, 2015
ATM, it wouldn't. Cmd help is buggy and needs to be addressed. I have put a card on board.
Currently, if you run juju help storage and juju help storage show, you will only get juju help storage...
I think that this wording (and other CLI commands that are structured under SuperCommands) will need to be re-worded..
anastasiamac
added some commits
Jan 28, 2015
added a commit
that referenced
this pull request
Jan 28, 2015
anastasiamac commentedJan 28, 2015
One test fails. Did not touch it :D
in ../cmd/juju
deploy_test.go:216:
c.Assert(err, jc.ErrorIsNil)
... value *params.Error = ¶ms.Error{"", "cannot add service "storage-block": no storage pool specifed and no default available for storage kind "block" for "data" storage"} ("cannot add service "storage-block": no storage pool specifed and no default available for storage kind "block" for "data" storage")