Route creation with oc new-app #16396

mjudeikis · 2017-09-16T19:43:28Z

Story: https://trello.com/c/Ve6T2QUL/1336-create-routes-from-oc-new-app-appcreation

Comments more than welcome as I'm not familiar with the codebase.

bparees · 2017-09-18T01:01:57Z

/unassign @smarterclayton
/unassign @juanvallejo
/assign
/assign @jim-minter

jim-minter

Great start @mangirdaz!

jim-minter · 2017-09-18T12:56:05Z

pkg/template/controller/readiness.go

@@ -133,6 +134,15 @@ func checkStatefulSetReadiness(oc client.Interface, obj runtime.Object) (bool, b
 	return ready, failed, nil
 }

+// checkRouteReadiness checks if host fiels was prepoplated already.


s/fiels/field/
s/prepoplated/prepopulated/

jim-minter · 2017-09-18T12:57:34Z

pkg/template/controller/readiness.go

@@ -133,6 +134,15 @@ func checkStatefulSetReadiness(oc client.Interface, obj runtime.Object) (bool, b
 	return ready, failed, nil
 }

+// checkRouteReadiness checks if host fiels was prepoplated already.
+func checkRouteReadiness(oc client.Interface, obj runtime.Object) (bool, bool, error) {


Add two tests to TestCheckReadiness in pkg/template/controller/readiness_test.go, showing a Route that will return !ready, and one that will return ready

jim-minter · 2017-09-18T12:59:53Z

pkg/template/controller/readiness.go

@@ -146,6 +156,7 @@ var readinessCheckers = map[schema.GroupKind]func(client.Interface, runtime.Obje
 	deployapi.Kind("DeploymentConfig"):       checkDeploymentConfigReadiness,
 	batch.Kind("Job"):                        checkJobReadiness,
 	apps.Kind("StatefulSet"):                 checkStatefulSetReadiness,
+	routeapi.Kind("Route"):                   checkRouteReadiness,


You need an additional line routeapi.LegacyKind("Route"): checkRouteReadiness, here. This will handle objects with apiVersion: v1, kind: Route. Your existing line only handles objects with apiVersion: route.openshift.io/v1, kind: Route.

jim-minter · 2017-09-18T13:04:48Z

pkg/oc/cli/cmd/newapp.go

@@ -373,6 +374,10 @@ func (o *NewAppOptions) RunNewApp() error {
 				hasMissingRepo = true
 				fmt.Fprintf(out, "%sWARNING: No Docker registry has been configured with the server. Automatic builds and deployments may not function.\n", indent)
 			}
+		case *routeapi.Route:
+			if len(t.Spec.Host) > 0 {
+				fmt.Fprintf(out, "%sThe Access application via route '%s' \n", indent, t.Spec.Host)


s/The Access application/Access your app/

jim-minter · 2017-09-18T13:44:07Z

pkg/oc/cli/cmd/newapp.go

 		fmt.Fprintf(out, "%sRun '%s %s' to view your app.\n", indent, o.BaseName, StatusRecommendedName)
 	}
 	return nil
 }

 type LogsForObjectFunc func(object, options runtime.Object, timeout time.Duration) (*restclient.Request, error)

+func containsRoute(item []runtime.Object) bool {


I think I'd remove this function and just set a flag variable if you find a Route when iterating result.List.Items in RunNewApp()

jim-minter · 2017-09-18T13:44:53Z

pkg/oc/cli/cmd/newapp.go

+	return false
+}
+
+func getAllServices(item []runtime.Object) []string {


func getServices(items []runtime.Object) []*kapi.Service would be a better signature

jim-minter · 2017-09-18T13:50:12Z

pkg/oc/cli/cmd/newapp.go

@@ -385,13 +390,45 @@ func (o *NewAppOptions) RunNewApp() error {
 			fmt.Fprintf(out, "%sTrack installation of %s with '%s logs %s'.\n", indent, installing[i].Name, o.BaseName, installing[i].Name)
 		}
 	case len(result.List.Items) > 0:
+		//if we dont find a route we give a message to expose it
+		if !containsRoute(result.List.Items) {
+			//we if dont have any route but have services - suggest commndas to expose those


jim-minter · 2017-09-18T13:52:15Z

pkg/oc/cli/cmd/newapp.go

+			//we if dont have any route but have services - suggest commndas to expose those
+			svc := getAllServices(result.List.Items)
+			if len(svc) > 0 {
+				fmt.Fprintf(out, "%sApplication is not exposed. You can expose it to outside world by executing one or all of the commands below \n", indent)


Not sure on the final wording for this: maybe the following is better?
"%sApp is not exposed. You can expose services to the outside world by executing one or more of the commands below:\n"

jim-minter · 2017-09-18T13:55:54Z

@mangirdaz also, can you see if you can get a couple of new tests together in test/cmd/newapp.sh ? For example something like os::cmd::expect_success_and_text 'oc new-app mysql --dry-run' 'Application is not exposed'

mjudeikis · 2017-09-20T11:14:31Z

@jim-minter fixed as per comments.

will add tests for new app sh script for testing. Was wondering how you test this :)

mjudeikis · 2017-09-20T12:41:52Z

/test cmd

jim-minter · 2017-09-20T15:52:20Z

pkg/oc/cli/cmd/newapp.go

+		for _, i := range result.List.Items {
+			switch i.(type) {
+			case *routeapi.Route:
+				containRoute = true


I meant to push this up to around line 377 to save the additional iteration - is that possible?

jim-minter · 2017-09-20T15:54:07Z

pkg/oc/cli/cmd/newapp.go

+func getServices(items []runtime.Object) []*kapi.Service {
+	var svc []*kapi.Service
+	for _, i := range items {
+		switch i.(type) {


no need to change it, but for reference, you can also do if s, ok := i.(*kapi.Service); ok { svc = append(svc, s) }

jim-minter · 2017-09-20T15:57:40Z

test/cmd/newapp.sh

@@ -36,6 +36,8 @@ os::cmd::expect_success_and_text 'oc new-app mysql --dry-run' 'tag "5.7" for "my
 # test deployments are created with the boolean flag and printed in the UI
 os::cmd::expect_success_and_text 'oc new-app mysql --dry-run --as-test' 'This image will be test deployed'
 os::cmd::expect_success_and_text 'oc new-app mysql -o yaml --as-test' 'test: true'
+os::cmd::expect_success_and_text 'oc new-app mysql --as-test' 'Application is not exposed'


I think that if you add --dry-run here, it'll save having to clean anything up in the test

Dry run does not work as we terminate all new-app stuff before we reach output phase. And it makes sense as we dont have what to "expose"

if !o.Action.Verbose() || o.Action.DryRun { return nil }

Ah - then do you need to add some os::cmd::expect_success 'oc delete all --selector="label=something"' lines?

Added one more new small template as reusing old ones was causing lot of false/positives.
+Added cleaning... This file is so sensitive... you touch one test and 5 starts failing :D

jim-minter · 2017-09-20T15:59:38Z

/retest

jim-minter · 2017-09-20T16:00:12Z

@bparees I think this is nearly there - want to take a look?

jim-minter · 2017-09-20T16:03:09Z

@mangirdaz you can/should squash your commits before this gets merged, use git rebase -i master and change the 2nd-last "pick"s to "fixup". If you accidentally mess things up, read up on git reflog, it's a lifesaver.

bparees · 2017-09-20T16:39:17Z

pkg/oc/cli/cmd/newapp.go

@@ -373,6 +374,10 @@ func (o *NewAppOptions) RunNewApp() error {
 				hasMissingRepo = true
 				fmt.Fprintf(out, "%sWARNING: No Docker registry has been configured with the server. Automatic builds and deployments may not function.\n", indent)
 			}
+		case *routeapi.Route:
+			if len(t.Spec.Host) > 0 {
+				fmt.Fprintf(out, "%sAccess your application via route '%s' \n", indent, t.Spec.Host)


If we find a route object it's possible Host will be empty because the template.Route didn't specify a value. (which means it'll get filled in by the server when the Route object is created). This is where we should really be polling the created object on the server until it has a valid Host value filled in, and then report that value (similar to how your added readiness check works).

Does this mean we will create blocking process here? Should we poll the server for let's say 5s and if we cant get it we either error or dont print it at all?

yes i think a cap on how long to wait is a good idea.

What about this? Sorry for stupid question. Stil trying to get my head around on different implementations styles around the codebase.

// getRouteHost will poll server for host field to be populated. With timout func getRouteHost(c *newcmd.AppConfig, r *routeapi.Route) (*routeapi.Route, error) { var route *routeapi.Route c1 := make(chan string, 1) go func() { time.Sleep(time.Second * RoutePollTimoutSeconds) c1 <- "Route creation timout" }() select { case res := <-c1: return nil, err.New(res) case <-time.After(time.Microsecond * 500): route, err := c.OSClient.Routes(r.Namespace).Get(r.Name, metav1.GetOptions{}) if err != nil { return nil, err } if route.Spec.Host != "" { return route, nil } } return route, nil }

check out wait.Poll as it's used elsewhere in the codebase.

bparees · 2017-09-21T17:12:25Z

pkg/oc/cli/cmd/newapp.go

+			if len(t.Spec.Host) > 0 {
+				containsRoute = true
+				var route *routeapi.Route
+				//check if route processing was complted and host field is prepopulated by router


s/compltd/completed/

bparees · 2017-09-21T17:13:36Z

pkg/oc/cli/cmd/newapp.go

+					if route.Spec.Host != "" {
+						return true, nil
+					}
+					return false, coreerror.New(fmt.Sprintf("Timout while polling route %s", t.Name))


this should be return false, nil

bparees · 2017-09-21T17:54:38Z

lgtm. @smarterclayton any final concerns?

jim-minter · 2017-09-21T18:06:56Z

pkg/oc/cli/cmd/newapp.go

 	"github.com/openshift/origin/pkg/util"
 )

 // NewAppRecommendedCommandName is the recommended command name.
 const NewAppRecommendedCommandName = "new-app"

+// RoutePollTimoutSeconds sets how long new-app command waits for route host to be prepopulated
+const RoutePollTimoutSeconds = 5


const RoutePollTimeout = 5 * time.Second

jim-minter · 2017-09-21T18:50:49Z

pkg/oc/cli/cmd/newapp.go

@@ -2,6 +2,7 @@ package cmd

 import (
 	"bytes"
+	coreerror "errors"


fyi: very rare in our codebase to rename core go imports, best avoided. We'll get rid of this import below

jim-minter · 2017-09-21T18:51:17Z

pkg/oc/cli/cmd/newapp.go

@@ -373,6 +379,27 @@ func (o *NewAppOptions) RunNewApp() error {
 				hasMissingRepo = true
 				fmt.Fprintf(out, "%sWARNING: No Docker registry has been configured with the server. Automatic builds and deployments may not function.\n", indent)
 			}
+		case *routeapi.Route:
+			if len(t.Spec.Host) > 0 {
+				containsRoute = true


move containsRoute = true outside the if statement

jim-minter · 2017-09-21T18:52:29Z

pkg/oc/cli/cmd/newapp.go

+				err := wait.PollImmediate(500*time.Millisecond, time.Second*RoutePollTimoutSeconds, func() (bool, error) {
+					route, err = config.OSClient.Routes(t.Namespace).Get(t.Name, metav1.GetOptions{})
+					if err != nil {
+						return false, coreerror.New(fmt.Sprintf("Error while polling route %s", t.Name))


fyi: fmt.Errorf == errors.New(fmt.Sprintf(...))
but here, just return the error without wrapping it -- you're wrapping it in line 398

bparees · 2017-09-22T13:29:21Z

/approve
( @jim-minter or @smarterclayton can merge this w/ a lgtm)

jim-minter · 2017-09-22T13:44:19Z

/lgtm
thanks @mangirdaz !!

openshift-merge-robot · 2017-09-22T13:44:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, jim-minter, mangirdaz

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/oc/OWNERS~~ [bparees]
~~pkg/template/OWNERS~~ [bparees]
~~test/cmd/OWNERS~~ [bparees]
~~test/testdata/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-09-22T15:23:03Z

Automatic merge from submit-queue

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2017

openshift-merge-robot assigned juanvallejo and smarterclayton Sep 16, 2017

openshift-ci-robot assigned bparees and jim-minter and unassigned smarterclayton and juanvallejo Sep 18, 2017

jim-minter suggested changes Sep 18, 2017

View reviewed changes

jim-minter reviewed Sep 20, 2017

View reviewed changes

bparees reviewed Sep 20, 2017

View reviewed changes

mjudeikis force-pushed the new-app-routes branch from ba9de15 to 517266e Compare September 20, 2017 21:45

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 20, 2017

mjudeikis force-pushed the new-app-routes branch from f95ba19 to 3a64066 Compare September 21, 2017 15:30

mjudeikis changed the title ~~[WIP] Route creation with oc new-app~~ Route creation with oc new-app Sep 21, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 21, 2017

bparees requested changes Sep 21, 2017

View reviewed changes

mjudeikis force-pushed the new-app-routes branch from 3a64066 to 0fbc994 Compare September 21, 2017 17:38

bparees approved these changes Sep 21, 2017

View reviewed changes

jim-minter suggested changes Sep 21, 2017

View reviewed changes

oc new-app expose message logic

9ef1268

mjudeikis force-pushed the new-app-routes branch from 0fbc994 to 9ef1268 Compare September 21, 2017 19:10

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2017

openshift-merge-robot merged commit 3618de2 into openshift:master Sep 22, 2017

Route creation with oc new-app #16396

Route creation with oc new-app #16396

Conversation

mjudeikis commented Sep 16, 2017

bparees commented Sep 18, 2017

jim-minter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter commented Sep 18, 2017

mjudeikis commented Sep 20, 2017 • edited Loading

mjudeikis commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter commented Sep 20, 2017

jim-minter commented Sep 20, 2017

jim-minter commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 22, 2017

jim-minter commented Sep 22, 2017

openshift-merge-robot commented Sep 22, 2017

openshift-merge-robot commented Sep 22, 2017

mjudeikis commented Sep 20, 2017 •

edited

Loading