The Savilerow project

Merge lp:~savilerow-team/savilerow/savvy-go-cli into lp:savilerow/savvy

  1. savvy-go-cli
  2. Merge into savvy
Proposed by Chris Wayne
Status: Merged
Merged at revision: 13
Proposed branch: lp:~savilerow-team/savilerow/savvy-go-cli
Merge into: lp:savilerow/savvy
Diff against target: 243 lines (+160/-4)
11 files modified
debian/control (+2/-1)
debian/rules (+3/-3)
savvy-tailor/add.go (+23/-0)
savvy-tailor/args.go (+11/-0)
savvy-tailor/build.go (+19/-0)
savvy-tailor/create.go (+19/-0)
savvy-tailor/deploy.go (+19/-0)
savvy-tailor/main.go (+3/-0)
savvy-tailor/set.go (+23/-0)
savvy-tailor/tailor.go (+19/-0)
savvy-tailor/test.go (+19/-0)
To merge this branch: bzr merge lp:~savilerow-team/savilerow/savvy-go-cli
Reviewer Review Type Date Requested Status
Jani Monoses (community) Approve
Alex Chiang (community) Approve
Review via email: mp+215262@code.launchpad.net

Description of the change

Implementing the savvy bits directly in go will be beneficial in that all of the code will be in one place, and it will be more platform independent. This MP is the first step to achieving that

To post a comment you must log in.
Revision history for this message
Alex Chiang (achiang) wrote :

35 +import (
36 + "github.com/jessevdk/go-flags"
37 +)

Can you double-check with serguisens about best practice for 3rd party imports?

General comment - what is the wisdom of splitting each verb into a separate file vs all in a single file? As for source code directory structure, should they all be at the top-level? Or would it make more sense to put the verbs into a subdir, such as "cmds"?

Many of the text descriptions of the verbs are not informative enough, or use inconsistent terminology vs. the Savvy docs. For instance:

26 + parser.AddCommand("add", "Add custom content", "Add custom content", &addCommand)

What is custom content? Is this pre-seeded content?

Another example:

66 +parser.AddCommand("build", "Build a custom package", "Build a custom package", &buildCommand)

What is a custom package? Is it a custom click app? Or are you talking about the custom tarball? Please make the descriptions here match the terminology used in the Savvy API docs.

180 +parser.AddCommand("tailor", "Run the customization wizard", "Run the customization editor", &tailorCommand)

Inconsistent use of wizard vs. editor. Also, this is seriously not a wizard:

http://en.wikipedia.org/wiki/Wizard_(software)

"a user interface type that presents a user with a sequence of dialog boxes that lead the user through a series of well-defined steps."

Tailor allows users to go in any order, and doesn't force a series of defined steps. Tailor should also be able to derive current state of tarball and the UI would be updated accordingly.

I think editor is closer to what we are trying to do here.

Final general comment -- would be useful to include text in the MP that describes what the go implementation is *replacing* so reviewers can compare old vs new implementations.

Thanks.

review: Needs Fixing
Revision history for this message
Chris Wayne (cwayne) wrote :

I'm not sure what you mean about best practice for 3rd party imports? ubuntu-device-flash args.go does the same exact import (in fact that's where I found this package)

Splitting up each verb into a separate file was done as suggested by the go-flags upstream documentation:

http://godoc.org/github.com/jessevdk/go-flags
"The most common, idiomatic way to implement commands is to define a global parser instance and implement each command in a separate file. These command files should define a go init function which calls AddCommand on the global parser."

The text descriptions are incomplete for sure, as I expect that there is still some discussion to be had re: which verbs to use and how to use them. The general purpose of this MP in specific was to get the parser, and well-known commands set up to iterate over.

66: That should have read custom tarball

100: I meant to change both to editor, I didn't realize I'd left one of them as 'wizard', apologies, will fix.

The go implementation is replacing the script at usr/bin/savvy within this branch.

Revision history for this message
Alex Chiang (achiang) wrote :

Re: 3rd party imports, didn't know if we should add the 3rd party package into the archive or not.

Also, what if it changes? A "no change" build for us might result in breakage if upstream changes.

What I'm asking here is to find out from sergio how he plans to deal with those issues.

--

Re: file per cmd -- ACK.

--

Re: descriptions -- ACK and +1 to iterate

--

Re: replacing script, so shouldn't this MP then remove the script?

review: Needs Information
Revision history for this message
Chris Wayne (cwayne) wrote :

So sergio actually packaged go-flags and included in universe already, (and since it's necessary for ubuntu-device-flash and ubuntu-emulator I'd say its less likely to be broken there).

Re: Replacing the script -- we shouldn't yet, as this doesn't actually have all (really any) of the functionality yet, this is just the starting point. I wanted to have smaller MPs, so planned to have this one with just the implementation of the cli parser, and then have MPs for adding functionalities to each command, rather than having one ginormous MP that would be more difficult to review.

Revision history for this message
Alex Chiang (achiang) wrote :

So if go-flags is in universe, does the import need to change? How does that work at the code level? Or is it going to be a packaging thing where the build-dep is specified to be the universe version?

ACK re: not replacing script.

review: Needs Information
Revision history for this message
Chris Wayne (cwayne) wrote :

AIUI the build-dep will be the universe version, as the package golang-go-flags-dev will be installed in the chroot, and the package will be found within the chroot's GOPATH, so it won't actually need to do the go get, therefore ensuring that we have the bits we expect.

lp:~savilerow-team/savilerow/savvy-go-cli updated
14. By Chris Wayne

Address MP comments, also forgot build-dep on go-flags

Revision history for this message
Alex Chiang (achiang) wrote :

This is good enough to go in as a first version to iterate against.

Thanks.

review: Approve
lp:~savilerow-team/savilerow/savvy-go-cli updated
15. By Chris Wayne

d/rules fixing so that it can build with multiple .go files

Revision history for this message
Jani Monoses (jani) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2014-03-18 15:12:15 +0000
3+++ debian/control 2014-04-11 13:16:20 +0000
4@@ -16,7 +16,8 @@
5 qtdeclarative5-private-dev,
6 libqt5opengl5-dev,
7 pkg-config,
8- git
9+ git,
10+ golang-go-flags-dev
11 Standards-Version: 3.9.5
12
13 Package: savvy
14
15=== modified file 'debian/rules'
16--- debian/rules 2014-03-18 11:18:43 +0000
17+++ debian/rules 2014-04-11 13:16:20 +0000
18@@ -2,16 +2,16 @@
19
20 export DH_OPTIONS
21 export DH_GOPKG := launchpad.net/savvy-tailor
22-export GOPATH=${CURDIR}/gopath
23+export GOPATH=${CURDIR}/gopath:/usr/share/gocode
24
25 %:
26 dh $@ --with bash-completion --buildsystem=golang --with=golang --fail-missing
27
28 override_dh_auto_build:
29- go build savvy-tailor/main.go
30+ go build -o savvy savvy-tailor/*.go
31
32 override_dh_auto_install:
33- cp main usr/bin/savvy-tailor
34+ cp savvy usr/bin/savvy-tailor
35 dh_auto_install -O--buildsystem=golang
36
37 override_dh_auto_test:
38
39=== added file 'savvy-tailor/add.go'
40--- savvy-tailor/add.go 1970-01-01 00:00:00 +0000
41+++ savvy-tailor/add.go 2014-04-11 13:16:20 +0000
42@@ -0,0 +1,23 @@
43+package main
44+
45+import (
46+ "fmt"
47+)
48+
49+type AddCommand struct {
50+}
51+
52+var addCommand AddCommand
53+
54+func (x *AddCommand) Execute(args []string) error {
55+ fmt.Println("add")
56+ return nil
57+}
58+
59+func (x *AddCommand) Usage() string {
60+ return "[CONTENT_TYPE] file"
61+}
62+
63+func init() {
64+ parser.AddCommand("add", "Add custom content", "Add custom content", &addCommand)
65+}
66
67=== added file 'savvy-tailor/args.go'
68--- savvy-tailor/args.go 1970-01-01 00:00:00 +0000
69+++ savvy-tailor/args.go 2014-04-11 13:16:20 +0000
70@@ -0,0 +1,11 @@
71+package main
72+
73+import (
74+ "github.com/jessevdk/go-flags"
75+)
76+
77+type arguments struct {
78+}
79+
80+var args arguments
81+var parser = flags.NewParser(&args, flags.Default)
82
83=== added file 'savvy-tailor/build.go'
84--- savvy-tailor/build.go 1970-01-01 00:00:00 +0000
85+++ savvy-tailor/build.go 2014-04-11 13:16:20 +0000
86@@ -0,0 +1,19 @@
87+package main
88+
89+import (
90+ "fmt"
91+)
92+
93+type BuildCommand struct {
94+}
95+
96+var buildCommand BuildCommand
97+
98+func (x *BuildCommand) Execute(args []string) error {
99+ fmt.Println("build")
100+ return nil
101+}
102+
103+func init() {
104+parser.AddCommand("build", "Build a custom package", "Build a custom package", &buildCommand)
105+}
106
107=== added file 'savvy-tailor/create.go'
108--- savvy-tailor/create.go 1970-01-01 00:00:00 +0000
109+++ savvy-tailor/create.go 2014-04-11 13:16:20 +0000
110@@ -0,0 +1,19 @@
111+package main
112+
113+import (
114+ "fmt"
115+)
116+
117+type CreateCommand struct {
118+}
119+
120+var createCommand CreateCommand
121+
122+func (x *CreateCommand) Execute(args []string) error {
123+ fmt.Println("create")
124+ return nil
125+}
126+
127+func init() {
128+parser.AddCommand("create", "Create a new custom project", "Create a new custom project", &createCommand)
129+}
130
131=== added file 'savvy-tailor/deploy.go'
132--- savvy-tailor/deploy.go 1970-01-01 00:00:00 +0000
133+++ savvy-tailor/deploy.go 2014-04-11 13:16:20 +0000
134@@ -0,0 +1,19 @@
135+package main
136+
137+import (
138+ "fmt"
139+)
140+
141+type DeployCommand struct {
142+}
143+
144+var deployCommand DeployCommand
145+
146+func (x *DeployCommand) Execute(args []string) error {
147+ fmt.Println("deplou")
148+ return nil
149+}
150+
151+func init() {
152+parser.AddCommand("deploy", "Deploy a built custom package to a device", "Deploy a built custom package to a device", &deployCommand)
153+}
154
155=== modified file 'savvy-tailor/main.go'
156--- savvy-tailor/main.go 2014-03-24 15:05:08 +0000
157+++ savvy-tailor/main.go 2014-04-11 13:16:20 +0000
158@@ -26,6 +26,9 @@
159 )
160
161 func main() {
162+ if _, err := parser.Parse(); err != nil {
163+ os.Exit(1)
164+ }
165 //set the APP_ID so that it stops complaining
166 os.Setenv("APP_ID", "com.ubuntu.savvy.tailor")
167 if err := run(); err != nil {
168
169=== added file 'savvy-tailor/set.go'
170--- savvy-tailor/set.go 1970-01-01 00:00:00 +0000
171+++ savvy-tailor/set.go 2014-04-11 13:16:20 +0000
172@@ -0,0 +1,23 @@
173+package main
174+
175+import (
176+ "fmt"
177+)
178+
179+type SetCommand struct {
180+}
181+
182+var setCommand SetCommand
183+
184+func (x *SetCommand) Execute(args []string) error {
185+ fmt.Println("set")
186+ return nil
187+}
188+
189+func (x *SetCommand) Usage() string {
190+ return "[SETTING] value"
191+}
192+
193+func init() {
194+parser.AddCommand("set", "Set a custom setting", "Set a custom setting", &setCommand)
195+}
196
197=== added file 'savvy-tailor/tailor.go'
198--- savvy-tailor/tailor.go 1970-01-01 00:00:00 +0000
199+++ savvy-tailor/tailor.go 2014-04-11 13:16:20 +0000
200@@ -0,0 +1,19 @@
201+package main
202+
203+import (
204+ "fmt"
205+)
206+
207+type TailorCommand struct {
208+}
209+
210+var tailorCommand TailorCommand
211+
212+func (x *TailorCommand) Execute(args []string) error {
213+ fmt.Println("tailor")
214+ return nil
215+}
216+
217+func init() {
218+parser.AddCommand("tailor", "Run the customization editor", "Run the customization editor", &tailorCommand)
219+}
220
221=== added file 'savvy-tailor/test.go'
222--- savvy-tailor/test.go 1970-01-01 00:00:00 +0000
223+++ savvy-tailor/test.go 2014-04-11 13:16:20 +0000
224@@ -0,0 +1,19 @@
225+package main
226+
227+import (
228+ "fmt"
229+)
230+
231+type TestCommand struct {
232+}
233+
234+var testCommand TestCommand
235+
236+func (x *TestCommand) Execute(args []string) error {
237+ fmt.Println("test")
238+ return nil
239+}
240+
241+func init() {
242+parser.AddCommand("test", "Test custom package", "Test custom package", &testCommand)
243+}

Subscribers

People subscribed via source and target branches

to all changes: