Display warning if env.yaml has urls for another push target but no v…

…alid urls for current push target (#3037) * validate urls during push Signed-off-by: Stephanie <stephanie.cao@ibm.com> * add integration test Signed-off-by: Stephanie <stephanie.cao@ibm.com> * use urlkind to validate url * fix unit test Signed-off-by: Stephanie <stephanie.cao@ibm.com> * update warning message * change the function name to avoid confusion Signed-off-by: Stephanie <stephanie.cao@ibm.com> * fix spelling mistake
redhat-developer · May 5, 2020 · d454457 · d454457
1 parent cd1a0db
commit d454457
Show file tree

Hide file tree

Showing 7 changed files with 88 additions and 40 deletions.
diff --git a/pkg/devfile/adapters/interface.go b/pkg/devfile/adapters/interface.go
@@ -1,6 +1,8 @@
 package adapters
 
-import "github.com/openshift/odo/pkg/devfile/adapters/common"
+import (
+	"github.com/openshift/odo/pkg/devfile/adapters/common"
+)
 
 type PlatformAdapter interface {
 	Push(parameters common.PushParameters) error

diff --git a/pkg/envinfo/envinfo.go b/pkg/envinfo/envinfo.go
@@ -32,6 +32,7 @@ type URLKind string
 const (
 	INGRESS URLKind = "ingress"
 	ROUTE   URLKind = "route"
+	DOCKER  URLKind = "docker"
 )
 
 // EnvInfoURL holds URL related information

diff --git a/pkg/odo/cli/component/devfile.go b/pkg/odo/cli/component/devfile.go
@@ -84,6 +84,7 @@ func (po *PushOptions) DevfilePush() (err error) {
 		DevfileRunCmd:   strings.ToLower(po.devfileRunCommand),
 	}
 
+	warnIfURLSInvalid(po.EnvSpecificInfo.GetURL())
 	// Start or update the component
 	err = devfileHandler.Push(pushParams)
 	if err != nil {
@@ -152,3 +153,28 @@ func (do *DeleteOptions) DevfileComponentDelete() error {
 	log.Successf("Successfully deleted component")
 	return nil
 }
+
+func warnIfURLSInvalid(url []envinfo.EnvInfoURL) {
+	// warnIfURLSInvalid checks if env.yaml contains a valide URL for the current pushtarget
+	// display a warning if no url(s) found for the current push target, but found url(s) for another push target
+	dockerURLExists := false
+	kubeURLExists := false
+	for _, element := range url {
+		if element.Kind == envinfo.DOCKER {
+			dockerURLExists = true
+		} else {
+			kubeURLExists = true
+		}
+	}
+	var urlOutput string
+	if len(url) > 1 {
+		urlOutput = "URLs"
+	} else {
+		urlOutput = "a URL"
+	}
+	if pushtarget.IsPushTargetDocker() && !dockerURLExists && kubeURLExists {
+		log.Warningf("Found %v defined for Kubernetes, but no valid URLs for Docker.", urlOutput)
+	} else if !pushtarget.IsPushTargetDocker() && !kubeURLExists && dockerURLExists {
+		log.Warningf("Found %v defined for Docker, but no valid URLs for Kubernetes.", urlOutput)
+	}
+}
diff --git a/pkg/odo/cli/url/create.go b/pkg/odo/cli/url/create.go
@@ -164,12 +164,11 @@ func (o *URLCreateOptions) Complete(name string, cmd *cobra.Command, args []stri
 			if err != nil {
 				return err
 			}
+			o.urlType = envinfo.DOCKER
 		}
 
 		if len(args) != 0 {
 			o.urlName = args[0]
-		} else if pushtarget.IsPushTargetDocker() {
-			o.urlName = "local-" + url.GetURLName(componentName, o.componentPort)
 		} else {
 			o.urlName = url.GetURLName(componentName, o.componentPort)
 		}
@@ -211,18 +210,8 @@ func (o *URLCreateOptions) Validate() (err error) {
 			return fmt.Errorf("host must be provided in order to create ingress")
 		}
 		for _, localURL := range o.EnvSpecificInfo.GetURL() {
-			// if current push target is Kube, but localURL contains ExposedPort
-			// if current push target is docker, but localURL contains Host
-			if o.urlName == localURL.Name &&
-				((!pushtarget.IsPushTargetDocker() && localURL.ExposedPort > 0) || (pushtarget.IsPushTargetDocker() && len(localURL.Host) > 0)) {
-				return fmt.Errorf("the url %s already exists for a different push target, please rerun the command using a different url name", o.urlName)
-			}
-			if !pushtarget.IsPushTargetDocker() {
-				curIngressDomain := fmt.Sprintf("%v.%v", o.urlName, o.host)
-				ingressDomainEnv := fmt.Sprintf("%v.%v", localURL.Name, localURL.Host)
-				if curIngressDomain == ingressDomainEnv {
-					return fmt.Errorf("the url %s already exists", curIngressDomain)
-				}
+			if o.urlName == localURL.Name {
+				return fmt.Errorf("the url %s already exists", o.urlName)
 			}
 		}
 	} else {

diff --git a/pkg/url/url.go b/pkg/url/url.go
@@ -541,14 +541,16 @@ func Push(client *occlient.Client, kClient *kclient.Client, parameters PushParam
 	if parameters.IsExperimentalModeEnabled && kClient != nil {
 		urls := parameters.EnvURLS
 		for _, url := range urls {
-			urlLOCAL[url.Name] = URL{
-				Spec: URLSpec{
-					Host:      url.Host,
-					Port:      url.Port,
-					Secure:    url.Secure,
-					tLSSecret: url.TLSSecret,
-					urlKind:   url.Kind,
-				},
+			if url.Kind != envinfo.DOCKER {
+				urlLOCAL[url.Name] = URL{
+					Spec: URLSpec{
+						Host:      url.Host,
+						Port:      url.Port,
+						Secure:    url.Secure,
+						tLSSecret: url.TLSSecret,
+						urlKind:   url.Kind,
+					},
+				}
 			}
 		}
 	} else {

diff --git a/pkg/url/url_test.go b/pkg/url/url_test.go
@@ -2,6 +2,9 @@ package url
 
 import (
 	"fmt"
+	"reflect"
+	"testing"
+
 	"github.com/kylelemons/godebug/pretty"
 	appsv1 "github.com/openshift/api/apps/v1"
 	routev1 "github.com/openshift/api/route/v1"
@@ -15,12 +18,11 @@ import (
 	"github.com/openshift/odo/pkg/testingutil"
 	"github.com/openshift/odo/pkg/url/labels"
 	"github.com/openshift/odo/pkg/util"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	extensionsv1 "k8s.io/api/extensions/v1beta1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"reflect"
-	"testing"
+
 	//"github.com/openshift/odo/pkg/util"
 	"github.com/openshift/odo/pkg/version"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -1329,7 +1331,7 @@ func TestPush(t *testing.T) {
 				}
 
 				if len(createdURLMap) != len(tt.createdURLs) {
-					t.Errorf("number of created urls is different, want: %d,got: %d", len(tt.deletedURLs), len(deletedURLMap))
+					t.Errorf("number of created urls is different, want: %d,got: %d", len(tt.createdURLs), len(createdURLMap))
 				}
 
 				if !tt.args.isRouteSupported {

diff --git a/tests/integration/devfile/docker/cmd_docker_devfile_url_test.go b/tests/integration/devfile/docker/cmd_docker_devfile_url_test.go
@@ -50,7 +50,7 @@ var _ = Describe("odo docker devfile url command tests", func() {
 
 			helper.CmdShouldPass("odo", "create", "nodejs", cmpName)
 			stdout = helper.CmdShouldPass("odo", "url", "create")
-			helper.MatchAllInOutput(stdout, []string{"local-" + cmpName + "-3000", "created for component"})
+			helper.MatchAllInOutput(stdout, []string{cmpName + "-3000", "created for component"})
 			stdout = helper.CmdShouldPass("odo", "push", "--devfile", "devfile.yaml")
 			Expect(stdout).To(ContainSubstring("Changes successfully pushed to component"))
 		})
@@ -66,26 +66,17 @@ var _ = Describe("odo docker devfile url command tests", func() {
 			helper.MatchAllInOutput(stdout, []string{url1, "created for component", "Changes successfully pushed to component"})
 		})
 
-		It("create with same url name under a different push target should fail", func() {
+		It("create with same url name should fail", func() {
 			var stdout string
 			url1 := helper.RandString(5)
-			url2 := helper.RandString(5)
 			helper.CmdShouldPass("git", "clone", "https://github.com/che-samples/web-nodejs-sample.git", projectDirPath)
 			helper.Chdir(projectDirPath)
 
 			helper.CmdShouldPass("odo", "create", "nodejs", cmpName)
 			helper.CmdShouldPass("odo", "url", "create", url1)
 
-			// url1 exists with push target set to docker, create url with same name should fail if push target is set to kube
-			helper.CmdShouldPass("odo", "preference", "set", "pushtarget", "kube", "-f")
-			stdout = helper.CmdShouldFail("odo", "url", "create", url1, "--host", "1.2.3.4.com")
-			Expect(stdout).To(ContainSubstring("already exists for a different push target"))
-
-			// url2 exists with push target set to kube, create url with same name should fail if push target is set to docker
-			helper.CmdShouldPass("odo", "url", "create", url2, "--host", "1.2.3.4.com")
-			helper.CmdShouldPass("odo", "preference", "set", "pushtarget", "docker", "-f")
-			stdout = helper.CmdShouldFail("odo", "url", "create", url2)
-			Expect(stdout).To(ContainSubstring("already exists for a different push target"))
+			stdout = helper.CmdShouldFail("odo", "url", "create", url1)
+			Expect(stdout).To(ContainSubstring("the url " + url1 + " already exists"))
 
 		})
 
@@ -106,4 +97,39 @@ var _ = Describe("odo docker devfile url command tests", func() {
 		})
 	})
 
+	Context("Switching pushtarget", func() {
+		It("switch from docker to kube, odo push should display warning", func() {
+			var stdout string
+			helper.CmdShouldPass("git", "clone", "https://github.com/che-samples/web-nodejs-sample.git", projectDirPath)
+			helper.Chdir(projectDirPath)
+
+			helper.CmdShouldPass("odo", "create", "nodejs", cmpName)
+			helper.CmdShouldPass("odo", "url", "create")
+
+			helper.CmdShouldPass("odo", "preference", "set", "pushtarget", "kube", "-f")
+			session := helper.CmdRunner("odo", "push", "--devfile", "devfile.yaml")
+			stdout = string(session.Wait().Out.Contents())
+			stderr := string(session.Wait().Err.Contents())
+			Expect(stderr).To(ContainSubstring("Found a URL defined for Docker, but no valid URLs for Kubernetes."))
+			Expect(stdout).To(ContainSubstring("Changes successfully pushed to component"))
+		})
+
+		It("switch from kube to docker, odo push should display warning", func() {
+			var stdout string
+			helper.CmdShouldPass("git", "clone", "https://github.com/che-samples/web-nodejs-sample.git", projectDirPath)
+			helper.Chdir(projectDirPath)
+			helper.CmdShouldPass("odo", "preference", "set", "pushtarget", "kube", "-f")
+			helper.CmdShouldPass("odo", "create", "nodejs", cmpName)
+			helper.CmdShouldPass("odo", "url", "create", "--host", "1.2.3.4.com", "--ingress")
+
+			helper.CmdShouldPass("odo", "preference", "set", "pushtarget", "docker", "-f")
+			session := helper.CmdRunner("odo", "push", "--devfile", "devfile.yaml")
+			stdout = string(session.Wait().Out.Contents())
+			stderr := string(session.Wait().Err.Contents())
+			Expect(stderr).To(ContainSubstring("Found a URL defined for Kubernetes, but no valid URLs for Docker."))
+			Expect(stdout).To(ContainSubstring("Changes successfully pushed to component"))
+
+		})
+	})
+
 })