upgrade-juju: improvements

environs/tools.go

 package environs
 
 import (
         "fmt"
         "launchpad.net/juju-core/environs/tools"
         "launchpad.net/juju-core/log"
         "launchpad.net/juju-core/state"
         "launchpad.net/juju-core/version"
 )
 
 // ToolsList holds a list of available tools.  Private tools take
 // precedence over public tools, even if they have a lower
 // version number.
 type ToolsList struct {
         Private tools.List
         Public  tools.List
 }
 
 // ListTools returns a ToolsList holding all the tools
 // available in the given environment that have the
 // given major version.
 func ListTools(env Environ, majorVersion int) (*ToolsList, error) {
         private, err := tools.ReadList(env.Storage(), majorVersion)
-        if err != nil && err != tools.ErrNoMatches {
+        if err := ignoreMissingTools(err); err != nil {
                 return nil, err
         }
         public, err := tools.ReadList(env.PublicStorage(), majorVersion)
-        if err != nil && err != tools.ErrNoMatches {
+        if err := ignoreMissingTools(err); err != nil {
                 return nil, err
         }
         return &ToolsList{
                 Private: private,
                 Public:  public,
         }, nil
 }
 
+func ignoreMissingTools(err error) error {

[rog, 2013/04/12 17:30:06]

this function name isn't greatly obvious.
i'd think it would be clearer to do:

func isMissingTools(err error) bool

then, above:

if !isMissingTools(err) {
     return err
}

but even better would be to avoid having an error for the case of no tools at
all, and say that an empty tools list is nothing more special than any other
empty slice.

[fwereade, 2013/04/14 01:35:08]

Done.

+        switch err {
+        case tools.ErrNoTools, tools.ErrNoMatches:

[rog, 2013/04/12 17:30:06]

again, no real reason for there to be a difference between ErrNoMatches and
ErrNoTools AFAICS

[fwereade, 2013/04/14 01:35:08]

FindAvailableTools

+                return nil
+        }
+        return err
+}
+
 // BestTools returns the most recent version
 // from the set of tools in the ToolsList that are
 // compatible with the given version, using flags
 // to determine possible candidates.
 // It returns nil if no such tools are found.
 func BestTools(list *ToolsList, vers version.Binary, flags ToolsSearchFlags) *state.Tools {
         if flags&CompatVersion == 0 {
                 panic("CompatVersion not implemented")
         }
         if tools := bestTools(list.Private, vers, flags); tools != nil {
                 return tools
         }
         return bestTools(list.Public, vers, flags)
 }
 
 // bestTools is like BestTools but operates on a single list of tools.
 func bestTools(toolsList []*state.Tools, vers version.Binary, flags ToolsSearchFlags) *state.Tools {
         var bestTools *state.Tools
         allowDev := vers.IsDev() || flags&DevVersion != 0
         allowHigher := flags&HighestVersion != 0
-        log.Debugf("finding best tools for version: %v", vers)
+        log.Debugf("environs: finding best tools for version %v (dev=%v)", vers, allowDev)

[dimitern, 2013/04/12 17:30:47]

environs/tools:

[fwereade, 2013/04/14 01:35:08]

wrong package

         for _, t := range toolsList {
-                log.Debugf("checking tools %v", t)
+                log.Debugf("environs: checking tools %v", t)

[dimitern, 2013/04/12 17:30:47]

ditto

                 if t.Major != vers.Major ||
                         t.Series != vers.Series ||
                         t.Arch != vers.Arch ||
                         !allowDev && t.IsDev() ||
                         !allowHigher && vers.Number.Less(t.Number) {
                         continue
                 }
                 if bestTools == nil || bestTools.Number.Less(t.Number) {
+                        log.Debugf("environs: new best tools found: %v", t)

[dimitern, 2013/04/12 17:30:47]

ditto

                         bestTools = t
                 }
         }
         return bestTools
 }
 
 // ToolsSearchFlags gives options when searching
 // for tools.
 type ToolsSearchFlags int
 
@@ skipping 24 matching lines @@
         toolsList, err := ListTools(env, vers.Major)
         if err != nil {
                 return nil, err
         }
         tools := BestTools(toolsList, vers, flags)
         if tools == nil {
                 return tools, &NotFoundError{fmt.Errorf("no compatible tools found")}
         }
         return tools, nil
 }
+
+// FindAvailableTools returns a tools.List containing all tools with a given
+// version number in the environment's private storage. If no tools are
+// present in private storage, it falls back to public storage; if no tools
+// are present there, it returns ErrNoTools. Tools from public and private
+// buckets are not mixed.

[dimitern, 2013/04/12 17:30:47]

also probably good to mention what does "mixed" mean here

[fwereade, 2013/04/14 01:35:08]

Rewritten

+func FindAvailableTools(environ Environ, majorVersion int) (tools.List, error) {
+        list, err := tools.ReadList(environ.Storage(), majorVersion)
+        if err == tools.ErrNoTools {

[fwereade, 2013/04/14 01:35:08]

FWIW, this here is the place we care about the distinction between ErrNoTools
and ErrNoMatches. But everything's a *NotFoundError now, as it should be, I
think.

+                list, err = tools.ReadList(environ.PublicStorage(), majorVersion)
+        }
+        return list, err
+}