Merge lp:~cmars/juju-core/local-repo-errmsg into lp:~go-bot/juju-core/trunk

Proposed by Casey Marshall on 2014-04-09
Status: Merged
Approved by: Casey Marshall on 2014-04-09
Approved revision: 2598
Merged at revision: 2596
Proposed branch: lp:~cmars/juju-core/local-repo-errmsg
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 73 lines (+20/-5)
3 files modified
cmd/juju/common.go (+8/-0)
cmd/juju/deploy.go (+8/-5)
provider/local/environprovider.go (+4/-0)
To merge this branch: bzr merge lp:~cmars/juju-core/local-repo-errmsg
Reviewer Review Type Date Requested Status
Juju Engineering 2014-04-09 Pending
Review via email: mp+215021@code.launchpad.net

Commit message

Error msg for local charm deploy missing series.

Add error message when unable to resolve series for local repository.
Update juju deploy doc message to describe behavior.

https://codereview.appspot.com/86160043/

Description of the change

Error msg for local charm deploy missing series.

Add error message when unable to resolve series for local repository.
Update juju deploy doc message to describe behavior.

https://codereview.appspot.com/86160043/

To post a comment you must log in.
Casey Marshall (cmars) wrote :

Reviewers: mp+215021_code.launchpad.net,

Message:
Please take a look.

Description:
Error msg for local charm deploy missing series.

Add error message when unable to resolve series for local repository.
Update juju deploy doc message to describe behavior.

https://code.launchpad.net/~cmars/juju-core/local-repo-errmsg/+merge/215021

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/86160043/

Affected files (+14, -4 lines):
   A [revision details]
   M cmd/juju/common.go
   M cmd/juju/deploy.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140409180231-hnxao50offx5c4c9
+New revision: <email address hidden>

Index: cmd/juju/common.go
=== modified file 'cmd/juju/common.go'
--- cmd/juju/common.go 2014-04-07 10:05:04 +0000
+++ cmd/juju/common.go 2014-04-09 19:31:16 +0000
@@ -4,6 +4,8 @@
  package main

  import (
+ "fmt"
+
   "launchpad.net/juju-core/charm"
   "launchpad.net/juju-core/cmd"
   "launchpad.net/juju-core/environs"
@@ -65,6 +67,12 @@
   }
   // Otherwise, look up the best supported series for this charm
   if series == "" {
+ if ref.Schema == "local" {
+ possibleUrl := &charm.URL{Reference: ref, Series: "precise"}
+ logger.Errorf(`The series is not specified in the environment
(default-series) or with the charm. Did you mean:
+ %s`, possibleUrl.String())
+ return nil, fmt.Errorf("cannot resolve series for charm: %q", ref)
+ }
    return client.ResolveCharm(ref)
   }
   return &charm.URL{Reference: ref, Series: series}, nil

Index: cmd/juju/deploy.go
=== modified file 'cmd/juju/deploy.go'
--- cmd/juju/deploy.go 2014-04-09 06:35:07 +0000
+++ cmd/juju/deploy.go 2014-04-09 19:31:16 +0000
@@ -49,12 +49,12 @@
  For cs:~user/precise/mysql
    cs:~user/mysql

-For local:precise/mysql
- local:mysql
-
-In all cases, a versioned charm URL will be expanded as expected (for
example,
+In these cases, a versioned charm URL will be expanded as expected (for
example,
  mysql-33 becomes cs:precise/mysql-33).

+However, for local charms, such as local:precise/mysql, one must specify
the
+series.
+
  <service name>, if omitted, will be derived from <charm name>.

  Constraints can be specified when using deploy by specifying the
--constraints

Curtis Hovey (sinzui) wrote :

Hi Casey.

I am not a reviewer, just an interested party.

https://codereview.appspot.com/86160043/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/86160043/diff/1/cmd/juju/deploy.go#newcode55
cmd/juju/deploy.go:55: However, for local charms, such as
local:precise/mysql, one must specify the
I see "assuming a current default series of" in the text above. That is
ambiguous. I din't know if that means the charm's default series or the
environment's default-series.

Maybe this phrasing is helpful
     However, for local charms, where the default-series is not specified
in the environment, one must specify the series.

Older versions of juju had default-series in the env template created by
"juju init". Many of my envs have a define default-series because juju
put it there. So new local users of the past few months are the common
victims of this change. Maybe the local template should a default-series
line commented out to help users discover the feature.

https://codereview.appspot.com/86160043/

2596. By Casey Marshall on 2014-04-09

Update deploy wording.

Casey Marshall (cmars) wrote :

Please take a look.

https://codereview.appspot.com/86160043/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/86160043/diff/1/cmd/juju/deploy.go#newcode55
cmd/juju/deploy.go:55: However, for local charms, such as
local:precise/mysql, one must specify the
On 2014/04/09 20:13:33, curtis wrote:
> I see "assuming a current default series of" in the text above. That
is
> ambiguous. I din't know if that means the charm's default series or
the
> environment's default-series.

Added a sentence to disambiguate.

> Maybe this phrasing is helpful
> However, for local charms, where the default-series is not
specified in the
> environment, one must specify the series.

Done.

> Older versions of juju had default-series in the env template created
by "juju
> init". Many of my envs have a define default-series because juju put
it there.
> So new local users of the past few months are the common victims of
this change.
> Maybe the local template should a default-series line commented out to
help
> users discover the feature.

Where does this template live? I see the templates for specific
providers, but none of them contain a commented default-series: as far
as I can tell.

My environment.yaml has a commented default-series, but it was generated
ages ago.

https://codereview.appspot.com/86160043/

Casey Marshall (cmars) wrote :

Updated 'juju deploy' doc, what do you think?

https://codereview.appspot.com/86160043/

2597. By Casey Marshall on 2014-04-09

Add default-series: example to local provider boilerplate.

Casey Marshall (cmars) wrote :
Tim Penhey (thumper) wrote :

LGTM

perhaps an example in the help doc?

https://codereview.appspot.com/86160043/diff/40001/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/86160043/diff/40001/cmd/juju/deploy.go#newcode58
cmd/juju/deploy.go:58: environment, one must specify the series.
Worthwhile adding an example here?

https://codereview.appspot.com/86160043/

2598. By Casey Marshall on 2014-04-09

Provide example of local charm URL in deploy doc.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmd/juju/common.go'
2--- cmd/juju/common.go 2014-04-09 16:36:12 +0000
3+++ cmd/juju/common.go 2014-04-09 22:31:41 +0000
4@@ -4,6 +4,8 @@
5 package main
6
7 import (
8+ "fmt"
9+
10 "launchpad.net/juju-core/charm"
11 "launchpad.net/juju-core/cmd"
12 "launchpad.net/juju-core/environs"
13@@ -65,6 +67,12 @@
14 }
15 // Otherwise, look up the best supported series for this charm
16 if series == "" {
17+ if ref.Schema == "local" {
18+ possibleUrl := &charm.URL{Reference: ref, Series: "precise"}
19+ logger.Errorf(`The series is not specified in the environment (default-series) or with the charm. Did you mean:
20+ %s`, possibleUrl.String())
21+ return nil, fmt.Errorf("cannot resolve series for charm: %q", ref)
22+ }
23 return client.ResolveCharm(ref)
24 }
25 return &charm.URL{Reference: ref, Series: series}, nil
26
27=== modified file 'cmd/juju/deploy.go'
28--- cmd/juju/deploy.go 2014-04-09 16:36:12 +0000
29+++ cmd/juju/deploy.go 2014-04-09 22:31:41 +0000
30@@ -39,8 +39,7 @@
31
32 const deployDoc = `
33 <charm name> can be a charm URL, or an unambiguously condensed form of it;
34-assuming a current default series of "precise", the following forms will be
35-accepted.
36+assuming a current series of "precise", the following forms will be accepted:
37
38 For cs:precise/mysql
39 mysql
40@@ -49,12 +48,16 @@
41 For cs:~user/precise/mysql
42 cs:~user/mysql
43
44-For local:precise/mysql
45- local:mysql
46+The current series is determined first by the default-series environment
47+setting, followed by the preferred series for the charm in the charm store.
48
49-In all cases, a versioned charm URL will be expanded as expected (for example,
50+In these cases, a versioned charm URL will be expanded as expected (for example,
51 mysql-33 becomes cs:precise/mysql-33).
52
53+However, for local charms, when the default-series is not specified in the
54+environment, one must specify the series. For example:
55+ local:precise/mysql
56+
57 <service name>, if omitted, will be derived from <charm name>.
58
59 Constraints can be specified when using deploy by specifying the --constraints
60
61=== modified file 'provider/local/environprovider.go'
62--- provider/local/environprovider.go 2014-04-02 11:35:49 +0000
63+++ provider/local/environprovider.go 2014-04-09 22:31:41 +0000
64@@ -262,6 +262,10 @@
65 #
66 # network-bridge: lxcbr0
67
68+ # The default series to deploy the state-server and charms on.
69+ #
70+ # default-series: precise
71+
72 `[1:]
73 }
74

Subscribers

People subscribed via source and target branches

to status/vote changes: