Command reconstruction - verb noun by JadeRedworth · Pull Request #291 · fnproject/cli

JadeRedworth · 2018-05-24T13:40:32Z

Reconstruction of the command structure within the CLI. It has been changed from a noun verb to verb noun structure, as described in issue #216

Folders added:

Commands - all the top-level commands
Test - holds all the test files
Object
- app
- call
- context
- log
- route
- server
Common
Run - all run related files

Open for suggestions: I don’t like the way the structure no handles apps and routes config.
fn list config apps I think it would be nicer to have fn list apps config but urfave will be expecting that 4th command and therefore fn list apps will not be valid.

^^ We went with fn config [app | route] <key> <value> to set a configuration.

Following changes to documentation and commands:
Fn #1031
Tutorials #87
Flow Lib Go #5
Fdk-ruby #13
Fdk-java #138

denismakogon · 2018-05-25T21:15:51Z

Would be nice to get #296 done as part of this change as well.

denismakogon · 2018-05-25T21:17:45Z

At least one of the tests is broken:

2018/05/25 12:41:31 Expected sdterr message (Unable to parse annotation value 'value'. Annotations values must be valid JSON strings.) not found in result: COMMAND: /home/circleci/go/src/github.com/fnproject/cli/fn create routes xpldnjob myroute --image foo/duffimage:0.0.1 --annotation test=value
INPUT:
RESULT: true

maybe a regression?

zootalures · 2018-06-01T14:44:57Z

Black box review:

can you accept both plural and singular of of apps and app , call/calls log/logs etc everywhere?
add ‘ls’ as an alias for list
Remove lambda command ? or put it at the bottom or hide it - it doesn't work anyway
fn push docs: “push function to Docker Hub” - > Push function to docker regsitry

Also fn set config app foo key value is a synonym for fn update app foo -c key=value
not sure it botheres me that much but just noticed.

The former seems a bit clunky - did you consider:

fn c[onfig[ure]] app foo key value ?

JadeRedworth · 2018-06-01T14:56:20Z

In regard to single vs plural, we can one command name and then multiple aliases. It will just change the help text to have the 3 options apps, app, a etc.. If this is fine then it can be added?

zootalures

Few nits here and there but good otherwise -

FWIW build server is a sort of esoteric command - and I don't think it should break the simplicity of fn build by making it fn build server and fn build function -

hence fn build-server or something

zootalures · 2018-06-01T16:42:47Z

 		// TODO: this doesn't seem to get called even when an error returns from a command, but maybe urfave is doing a non zero exit anyways? nope: https://github.com/urfave/cli/issues/610
 		fmt.Fprintf(os.Stderr, "ERROR: %v\n", err)
-		fmt.Fprintf(os.Stderr,"Client version: %s\n" , Version)
+		fmt.Fprintf(os.Stderr, "Client version: %s\n", commands.VersionCommand())


CopyPasta - here - this shoould be commands.Version

(or just Version once you've moved the version var up to /version.go)

zootalures · 2018-06-01T16:45:13Z

@@ -15,16 +15,17 @@ import (
 // Version of Fn CLI
 var Version = "0.4.106"


This is auto-set by in releases by the version bump tool - that expects a Version var in a version.goin the top level directory

Suggest creating version.go in ../ and just putting var Version = ".... in it as a global .

zootalures · 2018-06-01T16:48:09Z


-func getWd() string {
+type FnClient struct {
+	Client *fnclient.Fn


Don't see this being used anywhere - can it be deleted?

zootalures · 2018-06-01T16:48:22Z

@@ -138,7 +140,7 @@ func bumpVersion(funcfile *funcfile, t VType) (*funcfile, error) {
 // cleanImageName is intended to remove any trailing tag from the image name


Caps on doc

zootalures · 2018-06-01T16:55:56Z


 # start fn
-CONTAINER_ID=$($fn start -d)
+CONTAINER_ID=$($fn start server -d)


This is still just fn start right?

zootalures · 2018-06-01T16:57:04Z

@@ -1,7 +1,9 @@
-package main
+package test


This is a unit test on common - should be under common/bump_test.go

zootalures · 2018-06-01T16:58:03Z

 	tctx.WithFile("func.go", goFuncDotGo, 0644)

-	tctx.Fn("--verbose", "build").AssertFailed().AssertStderrContains("Dockerfile does not exist")
+	tctx.Fn("--verbose", "build", "function").AssertFailed().AssertStderrContains("Dockerfile does not exist")


This should be fn build shouldn't it?

zootalures · 2018-06-01T17:00:07Z

@@ -0,0 +1,27 @@
+package test


No unit tests in /test (sorry should have called it "e2e_test" or something - I think this test already exists in client/call_fn_test.go (my bad)

zootalures · 2018-06-01T17:01:30Z

 )

-type inputMap struct {
+type InputMap struct {


Dox on all the structs in this file now their public.

zootalures · 2018-06-01T17:06:49Z

 // • ‎then generate a main.go with extensions
 // • ‎compile, throw in another container like main dockerfile
-func (b *buildServerCmd) buildServer(c *cli.Context) error {
+func (b *buildServerCmd) Build(c *cli.Context) error {


Don't see this referenced anywhere

Suggest you put this as its own top-level command rather than under the build verb

fn build-server ?

zootalures · 2018-06-01T17:14:50Z

I did a quick scan and I think you pick up most uses of the ‘fn’ command that need changing in docs with the following grep:

 fgrep -rE "fn +(apps|routes|calls|logs|context|update) "  *  | grep -v \.git

client works filter through get sub commands dumping this here re order folder structure add oracle specific context file struct and move around more files refactor apps and calls add a test folder to hold test files fix cli_config_test.go file switch around commands in crud_test.go enable bash or zsh auto completion remove bash completion for this structure change Add bash completion for commands, add aliases for lambda command and fix tests. remove old app command add nicer start command message add <image> command to creating route add both singular and plural object commands rebase with master and fix client issue add singular and plural options rebase Move version to main package to allow for auto-bump by releases. Change build-server to its own command so it does affect fn build and clutter the command. Remove some unused code. Fix tests. update context readme

remove set command and replace with configure to set the configuration on an app/route

zootalures · 2018-06-05T13:34:16Z

-		Action: cmd.buildServer,
+		Name:     "build-server",
+		Usage:    "build custom fn server",
+		Category: "DEVELOPMENT COMMANDS",


I think this belongs more closely to SERVER COMMANDS

rikgibson

Can we get "help" and "version" in an "Other Commands" group?

rikgibson · 2018-06-05T14:24:44Z

I think the help text is quite sparse at this point but that can be worked on in conjunction with @michael-w-williams and shouldn't be a blocker in any way.

JadeRedworth · 2018-06-06T07:48:52Z

@rikgibson the help command comes with urfave so there is no way to add a category.
version can be moved to other commands but then it would be on its own?

Update CLI commands to reflect new CLI verb/noun structure to follow: fnproject/cli#291

Update CLI docs to reflect new CLI verb/noun structure to follow: fnproject/cli#291

JadeRedworth force-pushed the command-structure branch 3 times, most recently from 1a2a7cc to a8fb805 Compare May 29, 2018 08:40

JadeRedworth requested review from denismakogon, hibooboo2, msgodf, treeder and zootalures May 29, 2018 09:04

JadeRedworth force-pushed the command-structure branch from 3fedcb6 to 08f5ac7 Compare June 1, 2018 13:05

JadeRedworth force-pushed the command-structure branch from e78f6d3 to 011ad49 Compare June 1, 2018 16:10

zootalures requested changes Jun 1, 2018

View reviewed changes

JadeRedworth force-pushed the command-structure branch from 1a7adf2 to 516451a Compare June 4, 2018 08:36

move unit tests into their respective folders

3e3ddc3

remove set command and replace with configure to set the configuration on an app/route

JadeRedworth force-pushed the command-structure branch from 516451a to 3e3ddc3 Compare June 4, 2018 09:26

JadeRedworth added 3 commits June 4, 2018 11:07

add comments to exported functions

e5841d3

Comment on exported code

ddf834d

remove redundant comments

9814209

JadeRedworth requested a review from rikgibson June 5, 2018 11:18

JadeRedworth added 2 commits June 5, 2018 14:15

few capital letter edits

d62f22d

move config from catergory development commands to management commands

2e10a10

zootalures approved these changes Jun 5, 2018

View reviewed changes

zootalures reviewed Jun 5, 2018

View reviewed changes

move build server from development commands catergory to server commands

bd87cd2

rikgibson reviewed Jun 5, 2018

View reviewed changes

refactor some spelling

26d43c6

JadeRedworth force-pushed the command-structure branch from 2336fcb to 26d43c6 Compare June 6, 2018 07:50

JadeRedworth merged commit f83f292 into fnproject:master Jun 6, 2018

JadeRedworth added a commit to fnproject/fdk-ruby that referenced this pull request Jun 7, 2018

Merge pull request #13 from fnproject/update-commands

3b8a07c

Update CLI commands to reflect new CLI verb/noun structure to follow: fnproject/cli#291

JadeRedworth added a commit to fnproject/flow-lib-go that referenced this pull request Jun 7, 2018

Merge pull request #5 from fnproject/update-commands

7adf4de

Update CLI commands to reflect new CLI verb/noun structure to follow: fnproject/cli#291

JadeRedworth added a commit to fnproject/tutorials that referenced this pull request Jun 7, 2018

Merge pull request #87 from fnproject/update-new-command-structure

722aa7c

Update CLI docs to reflect new CLI verb/noun structure to follow: fnproject/cli#291

		@@ -15,16 +15,17 @@ import (
		// Version of Fn CLI
		var Version = "0.4.106"

		@@ -138,7 +140,7 @@ func bumpVersion(funcfile funcfile, t VType) (funcfile, error) {
		// cleanImageName is intended to remove any trailing tag from the image name

Conversation

JadeRedworth commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

denismakogon commented May 25, 2018

Uh oh!

denismakogon commented May 25, 2018

Uh oh!

zootalures commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JadeRedworth commented Jun 1, 2018

Uh oh!

zootalures left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zootalures commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rikgibson left a comment

Choose a reason for hiding this comment

Uh oh!

rikgibson commented Jun 5, 2018

Uh oh!

JadeRedworth commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JadeRedworth commented May 24, 2018 •

edited

Loading

zootalures commented Jun 1, 2018 •

edited

Loading

zootalures left a comment •

edited

Loading

zootalures commented Jun 1, 2018 •

edited

Loading