juju-core

Merge lp:~fwereade/juju-core/fix-1129319 into lp:~go-bot/juju-core/trunk

  1. fix-1129319
  2. Merge into trunk
Proposed by William Reade
Status: Merged
Approved by: William Reade
Approved revision: no longer in the source branch.
Merged at revision: 1307
Proposed branch: lp:~fwereade/juju-core/fix-1129319
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 51 lines (+17/-2)
3 files modified
charm/charm_test.go (+0/-1)
charm/repo.go (+7/-1)
charm/repo_test.go (+10/-0)
To merge this branch: bzr merge lp:~fwereade/juju-core/fix-1129319
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+169625@code.launchpad.net

Commit message

charm: local repos follow symlinks in series dirs

fixes lp:1129319

https://codereview.appspot.com/10302043/

Description of the change

charm: local repos follow symlinks in series dirs

fixes lp:1129319

https://codereview.appspot.com/10302043/

To post a comment you must log in.
Revision history for this message
William Reade (fwereade) wrote :

Reviewers: mp+169625_code.launchpad.net,

Message:
Please take a look.

Description:
charm: local repos follow symlinks in series dirs

fixes lp:1129319

https://code.launchpad.net/~fwereade/juju-core/fix-1129319/+merge/169625

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M charm/charm_test.go
   M charm/repo.go
   M charm/repo_test.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-20130614080827-ro5z9b8h1i5k37ow
+New revision: <email address hidden>

Index: charm/charm_test.go
=== modified file 'charm/charm_test.go'
--- charm/charm_test.go 2013-05-02 15:55:42 +0000
+++ charm/charm_test.go 2013-06-15 14:46:57 +0000
@@ -71,7 +71,6 @@
   switch f := f.(type) {
   case *charm.Bundle:
    c.Assert(f.Path, Equals, path)
-
   case *charm.Dir:
    c.Assert(f.Path, Equals, path)
   }

Index: charm/repo.go
=== modified file 'charm/repo.go'
--- charm/repo.go 2013-06-10 20:50:42 +0000
+++ charm/repo.go 2013-06-15 14:46:57 +0000
@@ -295,11 +295,21 @@
   return &NotFoundError{fmt.Sprintf("charm not found in %q: %s", repoPath,
curl)}
  }

-func mightBeCharm(info os.FileInfo) bool {
+func mightBeCharm(chPath string, info os.FileInfo) (bool, error) {
+ repoName := info.Name()
+ if info.Mode()&os.ModeSymlink != 0 {
+ var err error
+ if chPath, err = os.Readlink(chPath); err != nil {
+ return false, err
+ }
+ if info, err = os.Stat(chPath); err != nil {
+ return false, err
+ }
+ }
   if info.IsDir() {
- return !strings.HasPrefix(info.Name(), ".")
+ return !strings.HasPrefix(repoName, "."), nil
   }
- return strings.HasSuffix(info.Name(), ".charm")
+ return strings.HasSuffix(repoName, ".charm"), nil
  }

  // Get returns a charm matching curl, if one exists. If curl has a
revision of
@@ -326,10 +336,12 @@
   }
   var latest Charm
   for _, info := range infos {
- if !mightBeCharm(info) {
+ chPath := filepath.Join(path, info.Name())
+ if ok, err := mightBeCharm(chPath, info); err != nil {
+ return nil, err
+ } else if !ok {
     continue
    }
- chPath := filepath.Join(path, info.Name())
    if ch, err := Read(chPath); err != nil {
     log.Warningf("charm: failed to load charm at %q: %s", chPath, err)
    } else if ch.Meta().Name == curl.Name {

Index: charm/repo_test.go
=== modified file 'charm/repo_test.go'
--- charm/repo_test.go 2013-06-11 23:32:05 +0000
+++ charm/repo_test.go 2013-06-15 14:46:57 +0000
@@ -531,3 +531,13 @@
   s.checkNotFoundErr(c, err, charmURL)
   c.Assert(c.GetTestLog(), Equals, "")
  }
+
+func (s *LocalRepoSuite) TestFindsSymlinks(c *C) {
+ realPath := testing.Charms.ClonedDirPath(c.MkDir(), "dummy")
+ linkPath := filepath.Join(s.seriesPath, "dummy")
+ err := os.Symlink(realPath, linkPath)
+ c.Assert(err, IsNil)
+ ch, err := s.repo.Get(charm.MustParseURL("local:series/dummy"))
+ c.Assert(err, IsNil)
+ checkDummy(c, ch, linkPath)
+}

Revision history for this message
Frank Mueller (themue) wrote :

LGTM, including Daves comments.

https://codereview.appspot.com/10302043/

Revision history for this message
William Reade (fwereade) wrote :

Please take a look.

https://codereview.appspot.com/10302043/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/10302043/diff/1/charm/repo.go#newcode299
charm/repo.go:299: repoName := info.Name()
On 2013/06/17 07:18:30, dfc wrote:
> move below the if block

The if block might change what info points to; what I care about here is
the name inside the series dir. I'm not going to get hung up on the name
of what's pointed to, just on whether what it is matches what we expect
from looking at the link name.

https://codereview.appspot.com/10302043/diff/1/charm/repo.go#newcode302
charm/repo.go:302: if chPath, err = os.Readlink(chPath); err != nil {
On 2013/06/17 07:18:30, dfc wrote:
> if chPath, err := ...

I need chPath just below.

https://codereview.appspot.com/10302043/diff/1/charm/repo.go#newcode305
charm/repo.go:305: if info, err = os.Stat(chPath); err != nil {
On 2013/06/17 07:18:30, dfc wrote:
> same

I need info just below.

...but I can do all this simpler. Bah.

https://codereview.appspot.com/10302043/

Revision history for this message
Frank Mueller (themue) wrote :

Nice, LGTM.

https://codereview.appspot.com/10302043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'charm/charm_test.go'
--- charm/charm_test.go 2013-05-02 15:55:42 +0000
+++ charm/charm_test.go 2013-06-19 12:33:45 +0000
@@ -71,7 +71,6 @@
71 switch f := f.(type) {71 switch f := f.(type) {
72 case *charm.Bundle:72 case *charm.Bundle:
73 c.Assert(f.Path, Equals, path)73 c.Assert(f.Path, Equals, path)
74
75 case *charm.Dir:74 case *charm.Dir:
76 c.Assert(f.Path, Equals, path)75 c.Assert(f.Path, Equals, path)
77 }76 }
7877
=== modified file 'charm/repo.go'
--- charm/repo.go 2013-06-10 20:50:42 +0000
+++ charm/repo.go 2013-06-19 12:33:45 +0000
@@ -326,10 +326,16 @@
326 }326 }
327 var latest Charm327 var latest Charm
328 for _, info := range infos {328 for _, info := range infos {
329 chPath := filepath.Join(path, info.Name())
330 if info.Mode()&os.ModeSymlink != 0 {
331 var err error
332 if info, err = os.Stat(chPath); err != nil {
333 return nil, err
334 }
335 }
329 if !mightBeCharm(info) {336 if !mightBeCharm(info) {
330 continue337 continue
331 }338 }
332 chPath := filepath.Join(path, info.Name())
333 if ch, err := Read(chPath); err != nil {339 if ch, err := Read(chPath); err != nil {
334 log.Warningf("charm: failed to load charm at %q: %s", chPath, err)340 log.Warningf("charm: failed to load charm at %q: %s", chPath, err)
335 } else if ch.Meta().Name == curl.Name {341 } else if ch.Meta().Name == curl.Name {
336342
=== modified file 'charm/repo_test.go'
--- charm/repo_test.go 2013-06-11 23:32:05 +0000
+++ charm/repo_test.go 2013-06-19 12:33:45 +0000
@@ -531,3 +531,13 @@
531 s.checkNotFoundErr(c, err, charmURL)531 s.checkNotFoundErr(c, err, charmURL)
532 c.Assert(c.GetTestLog(), Equals, "")532 c.Assert(c.GetTestLog(), Equals, "")
533}533}
534
535func (s *LocalRepoSuite) TestFindsSymlinks(c *C) {
536 realPath := testing.Charms.ClonedDirPath(c.MkDir(), "dummy")
537 linkPath := filepath.Join(s.seriesPath, "dummy")
538 err := os.Symlink(realPath, linkPath)
539 c.Assert(err, IsNil)
540 ch, err := s.repo.Get(charm.MustParseURL("local:series/dummy"))
541 c.Assert(err, IsNil)
542 checkDummy(c, ch, linkPath)
543}

Subscribers

People subscribed via source and target branches

to status/vote changes: