Added the list-resources subcmd. #6

Merged
merged 2 commits into from Mar 21, 2016

Conversation

Projects
None yet
4 participants
Contributor

kat-co commented Mar 21, 2016

No description provided.

Member

jujugui commented Mar 21, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmstore-client/13/
Test PASSed.

@@ -86,6 +90,12 @@ type csClient struct {
filler esform.Filler
}
+// SaveJAR calls save on the jar member variable. This follows the Law
+// of Demeter and allows csClient to meet interfaces.
+func (c *csClient) SaveJAR() error {
@ericsnowcurrently

ericsnowcurrently Mar 21, 2016

We really need to pull out the common code between this and core. (but not here)

+ formatTabular func(interface{}) ([]byte, error),
+ username,
+ password string,
+ charmID *charm.URL,
cmd/charm/charmcmd/listresources.go
+
+var info = cmd.Info{
+ Name: "list-resources",
+ Args: "<charm> [--channel <channel>]",
@ericsnowcurrently

ericsnowcurrently Mar 21, 2016

Only positional args need to be here, no?

@kat-co

kat-co Mar 21, 2016

Contributor

Corrected.

cmd/charm/charmcmd/listresources.go
+`,
+}
+
+type ListResourcesCharmstoreClient interface {
cmd/charm/charmcmd/listresources.go
+ SaveJAR() error
+}
+
+type NewCharmstoreClientFn func(*cmd.Context, string, string) (ListResourcesCharmstoreClient, error)
@natefinch

natefinch Mar 21, 2016

Contributor

Can you put names on the arguments? Otherwise I don't know what the two strings are for.

@kat-co

kat-co Mar 21, 2016

Contributor

Corrected.

cmd/charm/charmcmd/listresources.go
+ cmd.CommandBase
+ cmd.Output
+
+ NewCharmstoreClient NewCharmstoreClientFn
@kat-co

kat-co Mar 21, 2016

Contributor

Made this is private instead.

cmd/charm/charmcmd/listresources.go
+ password string
+}
+
+func (c *listResourcesCommand) Info() *cmd.Info {
@ericsnowcurrently

ericsnowcurrently Mar 21, 2016

doc comment (on each of the methods)

@kat-co

kat-co Mar 21, 2016

Contributor

Corrected.

cmd/charm/charmcmd/listresources.go
+ "launchpad.net/gnuflag"
+)
+
+var info = cmd.Info{
@ericsnowcurrently

ericsnowcurrently Mar 21, 2016

Consider using a more specific name.

@kat-co

kat-co Mar 21, 2016

Contributor

Corrected.

cmd/charm/charmcmd/listresources.go
+}
+
+func parseArgs(auth string, args []string) (string, string, *charm.URL, error) {
+ // The valid combination of arguments is 1 or 3.
@ericsnowcurrently

ericsnowcurrently Mar 21, 2016

This appears to be inaccurate.

@kat-co

kat-co Mar 21, 2016

Contributor

Holdover; removed.

cmd/charm/charmcmd/listresources.go
+func parseArgs(auth string, args []string) (string, string, *charm.URL, error) {
+ // The valid combination of arguments is 1 or 3.
+ if len(args) != 1 {
+ return "", "", nil, errgo.New("no charm ID specified")
@ericsnowcurrently

ericsnowcurrently Mar 21, 2016

The established pattern is to use cmd.CheckEmpty() after popping off the expected arg.

@kat-co

kat-co Mar 21, 2016

Contributor

Thanks, corrected.

cmd/charm/charmcmd/listresources.go
+ Args: "<charm> [--channel <channel>]",
+ Purpose: "display the resources for a charm in the charm store",
+ Doc: `
+This command will report the resources for a charm in the charm store.
@natefinch

natefinch Mar 21, 2016

Contributor

"This command reports..."

@kat-co

kat-co Mar 21, 2016

Contributor

Copy/pasted this from juju's analogue command.

cmd/charm/charmcmd/listresources.go
+This command will report the resources for a charm in the charm store.
+
+<charm> can be a charm URL, or an unambiguously condensed form of it,
+just like the deploy command. So the following forms will be accepted:
@natefinch

natefinch Mar 21, 2016

Contributor

I don't think we should reference the deploy command, since that's a juju command not a charm command.

@kat-co

kat-co Mar 21, 2016

Contributor

Good catch. Corrected.

Other than a few minor comments, LGTM.

+For cs:~user/trusty/mysql
+ cs:~user/mysql
+
+Where the series is not supplied, the series from your local host is used.
@natefinch

natefinch Mar 21, 2016

Contributor

Is this true? I don't see code that does that, but I may be missing something.

+ "launchpad.net/gnuflag"
+)
+
+type listResourcesSuite struct {
@natefinch

natefinch Mar 21, 2016

Contributor

William prefers exported suites, I forget why though.

Member

jujugui commented Mar 21, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/charmstore-client/14/
Test PASSed.

+ return c.Write(ctx, resources)
+}
+
+func parseArgs(auth string, args []string) (string, string, *charm.URL, error) {
@natefinch

natefinch Mar 21, 2016

Contributor

please name these return args, so it's clear what they represent

@kat-co

kat-co Mar 21, 2016

Contributor

I feel the function is small enough that it's clear.

Thanks for taking care of that. LGTM.

Contributor

kat-co commented Mar 21, 2016

:shipit:

Member

jujugui commented Mar 21, 2016

Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/charmstore-client-merge

jujugui added a commit that referenced this pull request Mar 21, 2016

Merge pull request #6 from kat-co/list-resources-subcmd
Added the list-resources subcmd.

@jujugui jujugui merged commit ae225a3 into juju:master Mar 21, 2016

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment