Merge pull request #21487 from vespa-engine/ean/application-test-zip-issues

Ean/application test zip issues

5 files changed, 266 insertions, 176 deletions

diff --git a/client/go/cmd/config.go b/client/go/cmd/config.go
index e6fd1d93c1a..30db45c2bd2 100644
--- a/client/go/cmd/config.go
+++ b/client/go/cmd/config.go
@@ -208,7 +208,7 @@ func (c *Config) UseAPIKey(system vespa.System, tenantName string) bool {
 	// TODO: Remove this when users have had time to migrate over to Auth0 device flow authentication
 	a, err := auth0.GetAuth0(c.AuthConfigPath(), system.Name, system.URL)
 	if err != nil || !a.HasSystem() {
-		fmt.Fprintln(stderr, "Defaulting to tenant API key is deprecated. Use Auth0 device flow: 'vespa auth login' instead")
+		printWarning("Defaulting to tenant API key is deprecated.", "Use Auth0 device flow: 'vespa auth login' instead")
 		return util.PathExists(c.APIKeyPath(tenantName))
 	}
 	return false
diff --git a/client/go/cmd/helpers.go b/client/go/cmd/helpers.go
index 9003a64b33b..61a3257b58c 100644
--- a/client/go/cmd/helpers.go
+++ b/client/go/cmd/helpers.go
@@ -31,6 +31,13 @@ func printSuccess(msg ...interface{}) {
 	log.Print(color.Green("Success: "), fmt.Sprint(msg...))
 }
 
+func printWarning(msg string, hints ...string) {
+	fmt.Fprintln(stderr, color.Yellow("Warning:"), msg)
+	for _, hint := range hints {
+		fmt.Fprintln(stderr, color.Cyan("Hint:"), hint)
+	}
+}
+
 func athenzPath(filename string) (string, error) {
 	userHome, err := os.UserHomeDir()
 	if err != nil {
diff --git a/client/go/cmd/prod.go b/client/go/cmd/prod.go
index 10fc9f92368..b30f2e8551d 100644
--- a/client/go/cmd/prod.go
+++ b/client/go/cmd/prod.go
@@ -152,14 +152,12 @@ $ vespa prod submit`,
 				"The application must be a Java maven project, or include basic HTTP tests under src/test/application/",
 				"See https://cloud.vespa.ai/en/getting-to-production")
 		}
-		// TODO: Always verify tests. Do it before packaging, when running Maven from this CLI.
-		if !pkg.IsZip() {
-			verifyTests(pkg.TestPath, target)
+		if err := verifyTests(pkg, target); err != nil {
+			return err
 		}
 		isCI := os.Getenv("CI") != ""
 		if !isCI {
-			fmt.Fprintln(stderr, color.Yellow("Warning:"), "We recommend doing this only from a CD job")
-			printErrHint(nil, "See https://cloud.vespa.ai/en/getting-to-production")
+			printWarning("We recommend doing this only from a CD job", "See https://cloud.vespa.ai/en/getting-to-production")
 		}
 		opts, err := getDeploymentOptions(cfg, pkg, target)
 		if err != nil {
@@ -375,11 +373,30 @@ func prompt(r *bufio.Reader, question, defaultAnswer string, validator func(inpu
 	return input, nil
 }
 
-func verifyTests(testsParent string, target vespa.Target) {
-	verifyTest(testsParent, "system-test", target, true)
-	verifyTest(testsParent, "staging-setup", target, true)
-	verifyTest(testsParent, "staging-test", target, true)
-	verifyTest(testsParent, "production-test", target, false)
+func verifyTests(app vespa.ApplicationPackage, target vespa.Target) error {
+	// TODO: system-test, staging-setup and staging-test should be required if the application
+	//       does not have any Java tests.
+	suites := map[string]bool{
+		"system-test":     false,
+		"staging-setup":   false,
+		"staging-test":    false,
+		"production-test": false,
+	}
+	testPath := app.TestPath
+	if app.IsZip() {
+		path, err := app.Unzip(true)
+		if err != nil {
+			return err
+		}
+		defer os.RemoveAll(path)
+		testPath = path
+	}
+	for suite, required := range suites {
+		if err := verifyTest(testPath, suite, target, required); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 func verifyTest(testsParent string, suite string, target vespa.Target, required bool) error {
diff --git a/client/go/vespa/application.go b/client/go/vespa/application.go
new file mode 100644
index 00000000000..80965987b66
--- /dev/null
+++ b/client/go/vespa/application.go
@@ -0,0 +1,231 @@
+package vespa
+
+import (
+	"archive/zip"
+	"errors"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	"github.com/vespa-engine/vespa/client/go/util"
+)
+
+type ApplicationPackage struct {
+	Path     string
+	TestPath string
+}
+
+func (ap *ApplicationPackage) HasCertificate() bool {
+	return ap.hasFile(filepath.Join("security", "clients.pem"), "security/clients.pem")
+}
+
+func (ap *ApplicationPackage) HasDeployment() bool { return ap.hasFile("deployment.xml", "") }
+
+func (ap *ApplicationPackage) hasFile(filename, zipName string) bool {
+	if zipName == "" {
+		zipName = filename
+	}
+	if ap.IsZip() {
+		r, err := zip.OpenReader(ap.Path)
+		if err != nil {
+			return false
+		}
+		defer r.Close()
+		for _, f := range r.File {
+			if f.Name == zipName {
+				return true
+			}
+		}
+		return false
+	}
+	return util.PathExists(filepath.Join(ap.Path, filename))
+}
+
+func (ap *ApplicationPackage) IsZip() bool { return isZip(ap.Path) }
+
+func (ap *ApplicationPackage) IsJava() bool {
+	if ap.IsZip() {
+		r, err := zip.OpenReader(ap.Path)
+		if err != nil {
+			return false
+		}
+		defer r.Close()
+		for _, f := range r.File {
+			if filepath.Ext(f.Name) == ".jar" {
+				return true
+			}
+		}
+		return false
+	}
+	return util.PathExists(filepath.Join(ap.Path, "pom.xml"))
+}
+
+func isZip(filename string) bool { return filepath.Ext(filename) == ".zip" }
+
+func zipDir(dir string, destination string) error {
+	if filepath.IsAbs(dir) {
+		message := "Path must be relative, but '" + dir + "'"
+		return errors.New(message)
+	}
+	if !util.PathExists(dir) {
+		message := "'" + dir + "' should be an application package zip or dir, but does not exist"
+		return errors.New(message)
+	}
+	if !util.IsDirectory(dir) {
+		message := "'" + dir + "' should be an application package dir, but is a (non-zip) file"
+		return errors.New(message)
+	}
+
+	file, err := os.Create(destination)
+	if err != nil {
+		message := "Could not create a temporary zip file for the application package: " + err.Error()
+		return errors.New(message)
+	}
+	defer file.Close()
+
+	w := zip.NewWriter(file)
+	defer w.Close()
+
+	walker := func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if info.IsDir() {
+			return nil
+		}
+		file, err := os.Open(path)
+		if err != nil {
+			return err
+		}
+		defer file.Close()
+
+		zippath, err := filepath.Rel(dir, path)
+		if err != nil {
+			return err
+		}
+		zipfile, err := w.Create(zippath)
+		if err != nil {
+			return err
+		}
+
+		_, err = io.Copy(zipfile, file)
+		if err != nil {
+			return err
+		}
+		return nil
+	}
+	return filepath.Walk(dir, walker)
+}
+
+func (ap *ApplicationPackage) zipReader(test bool) (io.ReadCloser, error) {
+	zipFile := ap.Path
+	if test {
+		zipFile = ap.TestPath
+	}
+	if !ap.IsZip() {
+		tempZip, err := ioutil.TempFile("", "vespa")
+		if err != nil {
+			return nil, fmt.Errorf("could not create a temporary zip file for the application package: %w", err)
+		}
+		defer func() {
+			tempZip.Close()
+			os.Remove(tempZip.Name())
+			// TODO: Caller must remove temporary file
+		}()
+		if err := zipDir(zipFile, tempZip.Name()); err != nil {
+			return nil, err
+		}
+		zipFile = tempZip.Name()
+	}
+	f, err := os.Open(zipFile)
+	if err != nil {
+		return nil, fmt.Errorf("could not open application package at %s: %w", ap.Path, err)
+	}
+	return f, nil
+}
+
+func (ap *ApplicationPackage) Unzip(test bool) (string, error) {
+	if !ap.IsZip() {
+		return "", fmt.Errorf("can't unzip a package that is a directory structure")
+	}
+	cleanTemp := true
+	tmp, err := os.MkdirTemp(os.TempDir(), "vespa-test-pkg")
+	if err != nil {
+		return "", err
+	}
+	defer func() {
+		if cleanTemp {
+			os.RemoveAll(tmp)
+		}
+	}()
+	path := ap.Path
+	if test {
+		path = ap.TestPath
+	}
+	f, err := zip.OpenReader(path)
+	if err != nil {
+		return "", err
+	}
+	defer f.Close()
+	for _, f := range f.File {
+		dst := filepath.Join(tmp, f.Name)
+		if f.FileInfo().IsDir() {
+			if err := os.Mkdir(dst, f.FileInfo().Mode()); err != nil {
+				return "", err
+			}
+			continue
+		}
+		if err := copyFile(f, dst); err != nil {
+			return "", fmt.Errorf("copyFile: %w", err)
+		}
+
+	}
+	cleanTemp = false
+	return tmp, nil
+}
+
+func copyFile(src *zip.File, dst string) error {
+	from, err := src.Open()
+	if err != nil {
+		return err
+	}
+	defer from.Close()
+	to, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY, src.FileInfo().Mode())
+	if err != nil {
+		return err
+	}
+	defer to.Close()
+	_, err = io.Copy(to, from)
+	return err
+}
+
+// FindApplicationPackage finds the path to an application package from the zip file or directory zipOrDir.
+func FindApplicationPackage(zipOrDir string, requirePackaging bool) (ApplicationPackage, error) {
+	if isZip(zipOrDir) {
+		return ApplicationPackage{Path: zipOrDir}, nil
+	}
+	if util.PathExists(filepath.Join(zipOrDir, "pom.xml")) {
+		zip := filepath.Join(zipOrDir, "target", "application.zip")
+		if util.PathExists(zip) {
+			if testZip := filepath.Join(zipOrDir, "target", "application-test.zip"); util.PathExists(testZip) {
+				return ApplicationPackage{Path: zip, TestPath: testZip}, nil
+			}
+			return ApplicationPackage{Path: zip}, nil
+		}
+		if requirePackaging {
+			return ApplicationPackage{}, errors.New("pom.xml exists but no target/application.zip. Run mvn package first")
+		}
+	}
+	if path := filepath.Join(zipOrDir, "src", "main", "application"); util.PathExists(path) {
+		if testPath := filepath.Join(zipOrDir, "src", "test", "application"); util.PathExists(testPath) {
+			return ApplicationPackage{Path: path, TestPath: testPath}, nil
+		}
+		return ApplicationPackage{Path: path}, nil
+	}
+	if util.PathExists(filepath.Join(zipOrDir, "services.xml")) {
+		return ApplicationPackage{Path: zipOrDir}, nil
+	}
+	return ApplicationPackage{}, fmt.Errorf("could not find an application package source in '%s'", zipOrDir)
+}
diff --git a/client/go/vespa/deploy.go b/client/go/vespa/deploy.go
index 480408f3861..bc72ca1ba70 100644
--- a/client/go/vespa/deploy.go
+++ b/client/go/vespa/deploy.go
@@ -5,18 +5,14 @@
 package vespa
 
 import (
-	"archive/zip"
 	"bytes"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
 	"mime/multipart"
 	"net/http"
 	"net/url"
-	"os"
-	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -49,11 +45,6 @@ type DeploymentOptions struct {
 	Timeout            time.Duration
 }
 
-type ApplicationPackage struct {
-	Path     string
-	TestPath string
-}
-
 func (a ApplicationID) String() string {
 	return fmt.Sprintf("%s.%s.%s", a.Tenant, a.Application, a.Instance)
 }
@@ -83,105 +74,6 @@ func (d *DeploymentOptions) url(path string) (*url.URL, error) {
 	return url.Parse(service.BaseURL + path)
 }
 
-func (ap *ApplicationPackage) HasCertificate() bool {
-	return ap.hasFile(filepath.Join("security", "clients.pem"), "security/clients.pem")
-}
-
-func (ap *ApplicationPackage) HasDeployment() bool { return ap.hasFile("deployment.xml", "") }
-
-func (ap *ApplicationPackage) hasFile(filename, zipName string) bool {
-	if zipName == "" {
-		zipName = filename
-	}
-	if ap.IsZip() {
-		r, err := zip.OpenReader(ap.Path)
-		if err != nil {
-			return false
-		}
-		defer r.Close()
-		for _, f := range r.File {
-			if f.Name == zipName {
-				return true
-			}
-		}
-		return false
-	}
-	return util.PathExists(filepath.Join(ap.Path, filename))
-}
-
-func (ap *ApplicationPackage) IsZip() bool { return isZip(ap.Path) }
-
-func (ap *ApplicationPackage) IsJava() bool {
-	if ap.IsZip() {
-		r, err := zip.OpenReader(ap.Path)
-		if err != nil {
-			return false
-		}
-		defer r.Close()
-		for _, f := range r.File {
-			if filepath.Ext(f.Name) == ".jar" {
-				return true
-			}
-		}
-		return false
-	}
-	return util.PathExists(filepath.Join(ap.Path, "pom.xml"))
-}
-
-func (ap *ApplicationPackage) zipReader(test bool) (io.ReadCloser, error) {
-	zipFile := ap.Path
-	if test {
-		zipFile = ap.TestPath
-	}
-	if !ap.IsZip() {
-		tempZip, err := ioutil.TempFile("", "vespa")
-		if err != nil {
-			return nil, fmt.Errorf("could not create a temporary zip file for the application package: %w", err)
-		}
-		defer func() {
-			tempZip.Close()
-			os.Remove(tempZip.Name())
-		}()
-		if err := zipDir(zipFile, tempZip.Name()); err != nil {
-			return nil, err
-		}
-		zipFile = tempZip.Name()
-	}
-	f, err := os.Open(zipFile)
-	if err != nil {
-		return nil, fmt.Errorf("could not open application package at %s: %w", ap.Path, err)
-	}
-	return f, nil
-}
-
-// FindApplicationPackage finds the path to an application package from the zip file or directory zipOrDir.
-func FindApplicationPackage(zipOrDir string, requirePackaging bool) (ApplicationPackage, error) {
-	if isZip(zipOrDir) {
-		return ApplicationPackage{Path: zipOrDir}, nil
-	}
-	if util.PathExists(filepath.Join(zipOrDir, "pom.xml")) {
-		zip := filepath.Join(zipOrDir, "target", "application.zip")
-		if util.PathExists(zip) {
-			testZip := filepath.Join(zipOrDir, "target", "application-test.zip")
-			return ApplicationPackage{Path: zip, TestPath: testZip}, nil
-		}
-		if requirePackaging {
-			return ApplicationPackage{}, errors.New("pom.xml exists but no target/application.zip. Run mvn package first")
-		}
-	}
-	if util.PathExists(filepath.Join(zipOrDir, "src", "main", "application")) {
-		if util.PathExists(filepath.Join(zipOrDir, "src", "test", "application")) {
-			return ApplicationPackage{Path: filepath.Join(zipOrDir, "src", "main", "application"),
-				TestPath: filepath.Join(zipOrDir, "src", "test", "application")}, nil
-		}
-		return ApplicationPackage{Path: filepath.Join(zipOrDir, "src", "main", "application")}, nil
-	}
-	if util.PathExists(filepath.Join(zipOrDir, "services.xml")) {
-		return ApplicationPackage{Path: zipOrDir}, nil
-	}
-	return ApplicationPackage{}, errors.New("Could not find an application package source in '" + zipOrDir + "'")
-}
-
 func ApplicationFromString(s string) (ApplicationID, error) {
 	parts := strings.Split(s, ".")
 	if len(parts) != 3 {
@@ -407,63 +299,6 @@ func checkResponse(req *http.Request, response *http.Response, serviceDescriptio
 	return nil
 }
 
-func isZip(filename string) bool { return filepath.Ext(filename) == ".zip" }
-
-func zipDir(dir string, destination string) error {
-	if filepath.IsAbs(dir) {
-		message := "Path must be relative, but '" + dir + "'"
-		return errors.New(message)
-	}
-	if !util.PathExists(dir) {
-		message := "'" + dir + "' should be an application package zip or dir, but does not exist"
-		return errors.New(message)
-	}
-	if !util.IsDirectory(dir) {
-		message := "'" + dir + "' should be an application package dir, but is a (non-zip) file"
-		return errors.New(message)
-	}
-
-	file, err := os.Create(destination)
-	if err != nil {
-		message := "Could not create a temporary zip file for the application package: " + err.Error()
-		return errors.New(message)
-	}
-	defer file.Close()
-
-	w := zip.NewWriter(file)
-	defer w.Close()
-
-	walker := func(path string, info os.FileInfo, err error) error {
-		if err != nil {
-			return err
-		}
-		if info.IsDir() {
-			return nil
-		}
-		file, err := os.Open(path)
-		if err != nil {
-			return err
-		}
-		defer file.Close()
-
-		zippath, err := filepath.Rel(dir, path)
-		if err != nil {
-			return err
-		}
-		zipfile, err := w.Create(zippath)
-		if err != nil {
-			return err
-		}
-
-		_, err = io.Copy(zipfile, file)
-		if err != nil {
-			return err
-		}
-		return nil
-	}
-	return filepath.Walk(dir, walker)
-}
-
 // Returns the error message in the given JSON, or the entire content if it could not be extracted
 func extractError(reader io.Reader) string {
 	responseData, _ := ioutil.ReadAll(reader)