From fe845e188666a6d33d99b84a92c1c74b7b9cf455 Mon Sep 17 00:00:00 2001 From: C Cirello Date: Tue, 22 Nov 2016 00:27:48 +0100 Subject: [PATCH] fn: improve UX (#325) * fn: make UX more consistent with regards to app name position * fn: improve detection of missing routes * fn: fix update operations - No longer delete-than-add for configuration updates - Path cleaning before most of routes operations --- api/server/routes_delete.go | 13 +++- api/server/routes_get.go | 3 +- api/server/routes_test.go | 7 +- api/server/routes_update.go | 3 +- docs/swagger.yml | 41 +++++++++++- fn/apps.go | 77 ++++++++++++--------- fn/glide.lock | 6 +- fn/glide.yaml | 4 +- fn/publish.go | 2 +- fn/routes.go | 129 +++++++++++++++++++++--------------- 10 files changed, 189 insertions(+), 96 deletions(-) diff --git a/api/server/routes_delete.go b/api/server/routes_delete.go index a49b1ae1..08e02681 100644 --- a/api/server/routes_delete.go +++ b/api/server/routes_delete.go @@ -3,6 +3,7 @@ package server import ( "context" "net/http" + "path" "github.com/gin-gonic/gin" "github.com/iron-io/functions/api/models" @@ -14,10 +15,16 @@ func (s *Server) handleRouteDelete(c *gin.Context) { log := common.Logger(ctx) appName := c.Param("app") - routePath := c.Param("route") - err := Api.Datastore.RemoveRoute(appName, routePath) + routePath := path.Clean(c.Param("route")) - if err != nil { + route, err := Api.Datastore.GetRoute(appName, routePath) + if err != nil || route == nil { + log.Error(models.ErrRoutesNotFound) + c.JSON(http.StatusNotFound, simpleError(models.ErrRoutesNotFound)) + return + } + + if err := Api.Datastore.RemoveRoute(appName, routePath); err != nil { log.WithError(err).Debug(models.ErrRoutesRemoving) c.JSON(http.StatusInternalServerError, simpleError(models.ErrRoutesRemoving)) return diff --git a/api/server/routes_get.go b/api/server/routes_get.go index 4ab175da..6f98df29 100644 --- a/api/server/routes_get.go +++ b/api/server/routes_get.go @@ -3,6 +3,7 @@ package server import ( "context" "net/http" + "path" "github.com/Sirupsen/logrus" "github.com/gin-gonic/gin" @@ -15,7 +16,7 @@ func handleRouteGet(c *gin.Context) { log := common.Logger(ctx) appName := c.Param("app") - routePath := c.Param("route") + routePath := path.Clean(c.Param("route")) route, err := Api.Datastore.GetRoute(appName, routePath) if err != nil { diff --git a/api/server/routes_test.go b/api/server/routes_test.go index f056691e..ad02235d 100644 --- a/api/server/routes_test.go +++ b/api/server/routes_test.go @@ -64,7 +64,11 @@ func TestRouteDelete(t *testing.T) { tasks := mockTasksConduit() defer close(tasks) - router := testRouter(&datastore.Mock{}, &mqs.Mock{}, testRunner(t), tasks) + router := testRouter(&datastore.Mock{ + FakeRoutes: []*models.Route{ + &models.Route{AppName: "a", Path: "/myroute"}, + }, + }, &mqs.Mock{}, testRunner(t), tasks) for i, test := range []struct { path string @@ -74,6 +78,7 @@ func TestRouteDelete(t *testing.T) { }{ {"/v1/apps/a/routes", "", http.StatusTemporaryRedirect, nil}, {"/v1/apps/a/routes/myroute", "", http.StatusOK, nil}, + {"/v1/apps/a/routes/missing", "", http.StatusNotFound, nil}, } { _, rec := routerRequest(t, router, "DELETE", test.path, nil) diff --git a/api/server/routes_update.go b/api/server/routes_update.go index d86fd3cd..0c016799 100644 --- a/api/server/routes_update.go +++ b/api/server/routes_update.go @@ -3,6 +3,7 @@ package server import ( "context" "net/http" + "path" "github.com/gin-gonic/gin" "github.com/iron-io/functions/api/models" @@ -30,7 +31,7 @@ func handleRouteUpdate(c *gin.Context) { } wroute.Route.AppName = c.Param("app") - wroute.Route.Path = c.Param("route") + wroute.Route.Path = path.Clean(c.Param("route")) if wroute.Route.Image != "" { err = Api.Runner.EnsureImageExists(ctx, &runner.Config{ diff --git a/docs/swagger.yml b/docs/swagger.yml index 89b1046e..7e741de5 100644 --- a/docs/swagger.yml +++ b/docs/swagger.yml @@ -6,7 +6,7 @@ swagger: '2.0' info: title: IronFunctions description: The open source serverless platform. - version: "0.1.0" + version: "0.1.16" # the domain of the service host: "127.0.0.1:8080" # array of all schemes that your API supports @@ -184,6 +184,45 @@ paths: $ref: '#/definitions/Error' /apps/{app}/routes/{route}: + put: + summary: Update a Route + description: Update a route + tags: + - Routes + parameters: + - name: app + in: path + description: name of the app. + required: true + type: string + - name: route + in: path + description: route path. + required: true + type: string + - name: body + in: body + description: One route to post. + required: true + schema: + $ref: '#/definitions/RouteWrapper' + responses: + 201: + description: Route updated + schema: + $ref: '#/definitions/RouteWrapper' + 400: + description: One or more of the routes were invalid due to parameters being missing or invalid. + schema: + $ref: '#/definitions/Error' + 500: + description: Could not accept routes due to internal error. + schema: + $ref: '#/definitions/Error' + default: + description: Unexpected error + schema: + $ref: '#/definitions/Error' get: summary: Gets route by name description: Gets a route by name. diff --git a/fn/apps.go b/fn/apps.go index 1f3cb294..a60f0a78 100644 --- a/fn/apps.go +++ b/fn/apps.go @@ -20,14 +20,15 @@ func apps() cli.Command { return cli.Command{ Name: "apps", - Usage: "list apps", + Usage: "operate applications", ArgsUsage: "fn apps", - Action: a.list, Subcommands: []cli.Command{ { - Name: "create", - Usage: "create a new app", - Action: a.create, + Name: "create", + Aliases: []string{"c"}, + Usage: "create a new app", + ArgsUsage: "`app`", + Action: a.create, Flags: []cli.Flag{ cli.StringSliceFlag{ Name: "config", @@ -36,31 +37,45 @@ func apps() cli.Command { }, }, { - Name: "config", - Usage: "operate an application configuration set", - Action: a.configList, - Flags: []cli.Flag{ - cli.BoolFlag{ - Name: "shell", - Usage: "output in shell format", - }, - cli.BoolFlag{ - Name: "json", - Usage: "output in JSON format", - }, - }, + Name: "list", + Aliases: []string{"l"}, + Usage: "list all apps", + Action: a.list, + }, + { + Name: "config", + Usage: "operate an application configuration set", Subcommands: []cli.Command{ { - Name: "set", - Description: "store a configuration key for this application", - Usage: " ", - Action: a.configSet, + Name: "view", + Aliases: []string{"v"}, + Usage: "view all configuration keys for this app", + ArgsUsage: "`app`", + Action: a.configList, + Flags: []cli.Flag{ + cli.BoolFlag{ + Name: "shell,s", + Usage: "output in shell format", + }, + cli.BoolFlag{ + Name: "json,j", + Usage: "output in JSON format", + }, + }, + }, + { + Name: "set", + Aliases: []string{"s"}, + Usage: "store a configuration key for this application", + ArgsUsage: "`app` ", + Action: a.configSet, }, { - Name: "unset", - Description: "remove a configuration key for this application", - Usage: " ", - Action: a.configUnset, + Name: "unset", + Aliases: []string{"u"}, + Usage: "remove a configuration key for this application", + ArgsUsage: "`app` ", + Action: a.configUnset, }, }, }, @@ -69,7 +84,7 @@ func apps() cli.Command { } func (a *appsCmd) list(c *cli.Context) error { - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -95,7 +110,7 @@ func (a *appsCmd) create(c *cli.Context) error { return errors.New("error: app creating takes one argument, an app name") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -117,7 +132,7 @@ func (a *appsCmd) configList(c *cli.Context) error { return errors.New("error: app description takes one argument, an app name") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -157,7 +172,7 @@ func (a *appsCmd) configSet(c *cli.Context) error { return errors.New("error: application configuration setting takes three arguments: an app name, a key and a value") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -191,7 +206,7 @@ func (a *appsCmd) configUnset(c *cli.Context) error { return errors.New("error: application configuration setting takes three arguments: an app name, a key and a value") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } diff --git a/fn/glide.lock b/fn/glide.lock index 25bb7f0c..05ec0fb4 100644 --- a/fn/glide.lock +++ b/fn/glide.lock @@ -1,5 +1,5 @@ -hash: a7faac39f56e73fb3987d7a00062a52817432ba5e9620cc83dca15b75496f926 -updated: 2016-11-09T20:59:19.840009806+01:00 +hash: 30e4e556fec49a0c5bba030f272201870cb067d2b2c09d2e02240725e5bc09a2 +updated: 2016-11-21T19:58:04.868845654+01:00 imports: - name: github.com/aws/aws-sdk-go version: 90dec2183a5f5458ee79cbaf4b8e9ab910bc81a6 @@ -77,7 +77,7 @@ imports: - name: github.com/hashicorp/go-cleanhttp version: ad28ea4487f05916463e2423a55166280e8254b5 - name: github.com/iron-io/functions_go - version: 7f5bf75abece5380e916b594e17a552be365276d + version: 190cb886f033aac6d41e657eb91649b27cb52736 - name: github.com/iron-io/iron_go3 version: b50ecf8ff90187fc5fabccd9d028dd461adce4ee subpackages: diff --git a/fn/glide.yaml b/fn/glide.yaml index 773548d9..b923defa 100644 --- a/fn/glide.yaml +++ b/fn/glide.yaml @@ -7,8 +7,6 @@ import: subpackages: - bump - storage -- package: github.com/iron-io/functions_go - version: 7f5bf75abece5380e916b594e17a552be365276d - package: github.com/iron-io/iron_go3 subpackages: - config @@ -20,3 +18,5 @@ import: subpackages: - ssh/terminal - package: gopkg.in/yaml.v2 +- package: github.com/iron-io/functions_go + version: 190cb886f033aac6d41e657eb91649b27cb52736 diff --git a/fn/publish.go b/fn/publish.go index 40595fd0..ea9d79cd 100644 --- a/fn/publish.go +++ b/fn/publish.go @@ -88,7 +88,7 @@ func (p publishcmd) dockerpush(ff *funcfile) error { } func (p *publishcmd) route(path string, ff *funcfile) error { - if err := resetBasePath(&p.Configuration); err != nil { + if err := resetBasePath(p.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } diff --git a/fn/routes.go b/fn/routes.go index e2e875d4..3ee99323 100644 --- a/fn/routes.go +++ b/fn/routes.go @@ -26,71 +26,87 @@ func routes() cli.Command { return cli.Command{ Name: "routes", - Usage: "list routes", + Usage: "operate routes", ArgsUsage: "fn routes", - Action: r.list, Subcommands: []cli.Command{ { Name: "call", Usage: "call a route", - ArgsUsage: "appName /path", + ArgsUsage: "`app` /path", Action: r.call, Flags: runflags(), }, + { + Name: "list", + Aliases: []string{"l"}, + Usage: "list routes for `app`", + ArgsUsage: "`app`", + Action: r.list, + }, { Name: "create", - Usage: "create a route", - ArgsUsage: "appName /path image/name", + Aliases: []string{"c"}, + Usage: "create a route in an `app`", + ArgsUsage: "`app` /path image/name", Action: r.create, Flags: []cli.Flag{ cli.Int64Flag{ - Name: "memory", + Name: "memory,m", Usage: "memory in MiB", Value: 128, }, cli.StringFlag{ - Name: "type", + Name: "type,t", Usage: "route type - sync or async", Value: "sync", }, cli.StringSliceFlag{ - Name: "config", + Name: "config,c", Usage: "route configuration", }, }, }, { Name: "delete", - Usage: "delete a route", - ArgsUsage: "appName /path", + Aliases: []string{"d"}, + Usage: "delete a route from `app`", + ArgsUsage: "`app` /path", Action: r.delete, }, { - Name: "config", - Usage: "operate a route configuration set", - Action: r.configList, - Flags: []cli.Flag{ - cli.BoolFlag{ - Name: "shell", - Usage: "output in shell format", - }, - cli.BoolFlag{ - Name: "json", - Usage: "output in JSON format", - }, - }, + Name: "config", + Usage: "operate a route configuration set", Subcommands: []cli.Command{ { - Name: "set", - Description: "store a configuration key for this route", - Usage: " ", - Action: r.configSet, + Name: "view", + Aliases: []string{"v"}, + Usage: "view all configuration keys for this route", + ArgsUsage: "`app` /path", + Action: r.configList, + Flags: []cli.Flag{ + cli.BoolFlag{ + Name: "shell,s", + Usage: "output in shell format", + }, + cli.BoolFlag{ + Name: "json,j", + Usage: "output in JSON format", + }, + }, }, { - Name: "unset", - Description: "remove a configuration key for this route", - Usage: " ", - Action: r.configUnset, + Name: "set", + Aliases: []string{"s"}, + Usage: "store a configuration key for this route", + ArgsUsage: "`app` /path ", + Action: r.configSet, + }, + { + Name: "unset", + Aliases: []string{"u"}, + Usage: "remove a configuration key for this route", + ArgsUsage: "`app` /path ", + Action: r.configUnset, }, }, }, @@ -104,7 +120,7 @@ func call() cli.Command { return cli.Command{ Name: "call", Usage: "call a remote function", - ArgsUsage: "appName /path", + ArgsUsage: "`app` /path", Flags: runflags(), Action: r.call, } @@ -115,7 +131,7 @@ func (a *routesCmd) list(c *cli.Context) error { return errors.New("error: routes listing takes one argument, an app name") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -151,7 +167,7 @@ func (a *routesCmd) call(c *cli.Context) error { return errors.New("error: routes listing takes three arguments: an app name and a route") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -206,7 +222,7 @@ func (a *routesCmd) create(c *cli.Context) error { return errors.New("error: routes creation takes three arguments: an app name, a route path and an image") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -253,17 +269,22 @@ func (a *routesCmd) delete(c *cli.Context) error { return errors.New("error: routes listing takes three arguments: an app name and a path") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } appName := c.Args().Get(0) route := c.Args().Get(1) - _, err := a.AppsAppRoutesRouteDelete(appName, route) + + resp, err := a.AppsAppRoutesRouteDelete(appName, route) if err != nil { return fmt.Errorf("error deleting route: %v", err) } + if resp.StatusCode >= http.StatusBadRequest { + return fmt.Errorf("route not found: %s", route) + } + fmt.Println(route, "deleted") return nil } @@ -273,7 +294,7 @@ func (a *routesCmd) configList(c *cli.Context) error { return errors.New("error: route configuration description takes two arguments: an app name and a route") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -284,6 +305,10 @@ func (a *routesCmd) configList(c *cli.Context) error { return fmt.Errorf("error loading route information: %v", err) } + if msg := wrapper.Error_.Message; msg != "" { + return errors.New(msg) + } + config := wrapper.Route.Config if len(config) == 0 { return errors.New("this route has no configurations") @@ -314,7 +339,7 @@ func (a *routesCmd) configSet(c *cli.Context) error { return errors.New("error: route configuration setting takes four arguments: an app name, a route, a key and a value") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -325,7 +350,11 @@ func (a *routesCmd) configSet(c *cli.Context) error { wrapper, _, err := a.AppsAppRoutesRouteGet(appName, route) if err != nil { - return fmt.Errorf("error creating app: %v", err) + return fmt.Errorf("error loading route: %v", err) + } + + if msg := wrapper.Error_.Message; msg != "" { + return errors.New(msg) } config := wrapper.Route.Config @@ -337,11 +366,7 @@ func (a *routesCmd) configSet(c *cli.Context) error { config[key] = value wrapper.Route.Config = config - if _, err := a.AppsAppRoutesRouteDelete(appName, route); err != nil { - return fmt.Errorf("error deleting to force update route: %v", err) - } - - if _, _, err := a.AppsAppRoutesPost(appName, *wrapper); err != nil { + if _, _, err := a.AppsAppRoutesRoutePut(appName, route, *wrapper); err != nil { return fmt.Errorf("error updating route configuration: %v", err) } @@ -354,7 +379,7 @@ func (a *routesCmd) configUnset(c *cli.Context) error { return errors.New("error: route configuration setting takes four arguments: an app name, a route and a key") } - if err := resetBasePath(&a.Configuration); err != nil { + if err := resetBasePath(a.Configuration); err != nil { return fmt.Errorf("error setting endpoint: %v", err) } @@ -364,7 +389,11 @@ func (a *routesCmd) configUnset(c *cli.Context) error { wrapper, _, err := a.AppsAppRoutesRouteGet(appName, route) if err != nil { - return fmt.Errorf("error creating app: %v", err) + return fmt.Errorf("error loading app: %v", err) + } + + if msg := wrapper.Error_.Message; msg != "" { + return errors.New(msg) } config := wrapper.Route.Config @@ -380,11 +409,7 @@ func (a *routesCmd) configUnset(c *cli.Context) error { delete(config, key) wrapper.Route.Config = config - if _, err := a.AppsAppRoutesRouteDelete(appName, route); err != nil { - return fmt.Errorf("error deleting to force update route: %v", err) - } - - if _, _, err := a.AppsAppRoutesPost(appName, *wrapper); err != nil { + if _, _, err := a.AppsAppRoutesRoutePut(appName, route, *wrapper); err != nil { return fmt.Errorf("error updating route configuration: %v", err) }