address CR comments

expandybird/expansion/expansion.py

@@ -211,8 +211,7 @@ def ExpandTemplate(resource, imports, env, validate_schema=False):
 
   # source_file could be a short version of the template (say github short name)
   # so we need to potentially map this into the fully resolvable name.
-  if 'path' in imports[source_file]:
-    if imports[source_file]['path']:
+  if 'path' in imports[source_file] and imports[source_file]['path']:
       path = imports[source_file]['path']
 
   resource['imports'] = imports

manager/deployments.go

@@ -199,13 +199,13 @@ func putDeploymentHandlerFunc(w http.ResponseWriter, r *http.Request) {
 
 func getPathVariable(w http.ResponseWriter, r *http.Request, variable, handler string) (string, error) {
 	vars := mux.Vars(r)
-	retVariable, ok := vars[variable]
+	ret, ok := vars[variable]
 	if !ok {
 		e := fmt.Errorf("%s parameter not found in URL", variable)
 		util.LogAndReturnError(handler, http.StatusBadRequest, e, w)
 		return "", e
 	}
-	return retVariable, nil
+	return ret, nil
 }
 
 func getTemplate(w http.ResponseWriter, r *http.Request, handler string) *common.Template {

manager/manager/typeresolver.go

@@ -31,6 +31,8 @@ const (
 	schemaSuffix  = ".schema"
 )
 
+var re = regexp.MustCompile("github.com/(.*)/(.*)/(.*)/(.*):(.*)")
+
 // TypeResolver finds Types in a Configuration which aren't yet reduceable to an import file
 // or primitive, and attempts to replace them with a template from a URL.
 type TypeResolver interface {
@@ -40,7 +42,6 @@ type TypeResolver interface {
 type typeResolver struct {
 	getter  util.HTTPClient
 	maxUrls int
-	re      *regexp.Regexp
 	rp      registry.RegistryProvider
 }
 
@@ -53,7 +54,6 @@ func NewTypeResolver() TypeResolver {
 	client.Timeout = timeout
 	ret.getter = util.NewHTTPClient(3, client, util.NewSleeper())
 	ret.maxUrls = maxURLImports
-	ret.re = regexp.MustCompile("github.com/(.*)/(.*)/(.*)/(.*):(.*)")
 	ret.rp = &registry.DefaultRegistryProvider{}
 	return ret
 }
@@ -109,14 +109,14 @@ func (tr *typeResolver) ResolveTypes(config *common.Configuration, imports []*co
 
 	count := 0
 	for len(toFetch) > 0 {
-		//1. If short github URL, resolve to a download URL
-		//2. Fetch import URL. Exit if no URLs left
-		//3. Check/handle HTTP status
-		//4. Store results in all ImportFiles from that URL
-		//5. Check for the optional schema file at import URL + .schema
-		//6. Repeat 2,3 for schema file
-		//7. Add each schema import to fetch if not already done
-		//8. Mark URL done. Return to 1.
+		// 1. If short github URL, resolve to a download URL
+		// 2. Fetch import URL. Exit if no URLs left
+		// 3. Check/handle HTTP status
+		// 4. Store results in all ImportFiles from that URL
+		// 5. Check for the optional schema file at import URL + .schema
+		// 6. Repeat 2,3 for schema file
+		// 7. Add each schema import to fetch if not already done
+		// 8. Mark URL done. Return to 1.
 		if count >= tr.maxUrls {
 			return nil, resolverError(config,
 				fmt.Errorf("Number of imports exceeds maximum of %d", tr.maxUrls))
@@ -215,7 +215,7 @@ func (tr *typeResolver) MapFetchableURL(t string) (string, error) {
 // for example:
 // github.com/kubernetes/application-dm-templates/storage/redis:v1
 func (tr *typeResolver) ShortTypeToDownloadURL(template string) (string, error) {
-	m := tr.re.FindStringSubmatch(template)
+	m := re.FindStringSubmatch(template)
 	if len(m) != 6 {
 		return "", fmt.Errorf("Failed to parse short github url: %s", template)
 	}

manager/manager/typeresolver_test.go

@@ -18,7 +18,6 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
-	"regexp"
 	"strings"
 	"testing"
 
@@ -27,8 +26,6 @@ import (
 	"github.com/kubernetes/deployment-manager/common"
 )
 
-var re = regexp.MustCompile("github.com/(.*)/(.*)/(.*)/(.*):(.*)")
-
 type responseAndError struct {
 	err  error
 	code int
@@ -96,7 +93,6 @@ func (tgr *testGithubRegistry) List() ([]registry.Type, error) {
 
 func testUrlConversionDriver(c resolverTestCase, tests map[string]urlAndError, t *testing.T) {
 	r := &typeResolver{
-		re: re,
 		rp: c.registryProvider,
 	}
 	for in, expected := range tests {
@@ -115,7 +111,6 @@ func testDriver(c resolverTestCase, t *testing.T) {
 	r := &typeResolver{
 		getter:  g,
 		maxUrls: 5,
-		re:      re,
 		rp:      c.registryProvider,
 	}
 

registry/registry.go

@@ -48,6 +48,3 @@ type Registry interface {
 	// Get the download URL for a given template and version
 	GetURL(t Type) (string, error)
 }
-
-
-

util/templateutil.go

@@ -14,17 +14,16 @@ limitations under the License.
 package util
 
 import (
-	"strings"
-	"log"
+	"regexp"
 
 	"github.com/kubernetes/deployment-manager/common"
 )
 
+var re = regexp.MustCompile("github.com/(.*)/(.*)/(.*)/(.*):(.*)")
+
 // IsTemplate returns whether a given type is a template.
 func IsTemplate(t string, imports []*common.ImportFile) bool {
-	log.Printf("IsTemplate: %s : %+v", t, imports)
 	for _, imp := range imports {
-		log.Printf("Checking: %s", imp.Name)
 		if imp.Name == t {
 			return true
 		}
@@ -38,16 +37,5 @@ func IsTemplate(t string, imports []*common.ImportFile) bool {
 // for example:
 // github.com/kubernetes/application-dm-templates/storage/redis:v1
 func IsGithubShortType(t string) bool {
-	if !strings.HasPrefix(t, "github.com/") {
-		return false
-	}
-	s := strings.Split(t, "/")
-	if len(s) != 5 {
-		return false
-	}
-	v := strings.Split(s[4], ":")
-	if len(v) != 2 {
-		return false
-	}
-	return true
+	return re.MatchString(t)
 }