Code review comment for lp://qastaging/~pfalcon/linaro-android-build-tools/cmdline-client

Hey Paul,

following my comments, there's some typos that need fixing and a couple of suggestion/thoughts.
Marking as fix needed, but once you cleaned the typos it should be ready to go.

+Basic android-build.linaro.org command-line client
+==================================================
+
+This tool provides proof-of-concept command-line client for Linaro Android
+Build Jenkins master (android-build.linaro.org), as an alternative for
+existing Web UI interface. One particular feature the command-line client
+has and Web UI lacks is support for creating completely private builds (such
+builds are not availabel via Web UI at all). At the same time, this tool is
+so far in the proof of concept stage and provides only basic job management
+actions (namely, create a job and schedule its build). If you find this tools

s/this tools/this tool

+useful, please share you comments and suggestion using this bugtracker:

s/you comments/your comments

+
+https://bugs.launchpad.net/linaro-android-infrastructure
+
+to help its improvement and evolution.
+
+
+Quick start
+-----------
+
+1. You should have access to Jenkins at:
+
+http://android-build.linaro.org/jenkins/
+
+(Generally available for Linaro Android team members).
+
+2. Download android-build-client tool. As it is written to depend only

s/to depend/it depends

+on Python standard library, an easy way to get it is to download seperate
+file via Launchpad BZR viewer:
+
+wget <fill in link once branch lands>
+chmod +x android-build-client
+
+3. Look up your Jenkins API token by visiting
+https://android-build.linaro.org/jenkins/me/configure and clicking
+"Show API Token..." button.
+
+4. Run:
+
+./android-build-client authorize
+
+Enter you Jenkins username and API token. Note that this will cache these

s/you Jenkins/your Jenkins

+credentials in your home dir, so use this only on your personal well-protected
+workstation. The alternative is to use --user switch and input API key
+interactively.
+
+5. To create a new job, prepare an Android job config (see
+https://wiki.linaro.org/Platform/Android/LinaroAndroidBuildService for more
+info) in a file. Run:
+
+./android-build-client create <job_name> <job_config_file>
+
+Note that job name should conform to Android Build Service naming
+converntions, see documentation link above for more info. To create a job
+private for particular group, pass --private=<group> switch to create command.

s/job private/private job

+
+6. To schedule a build, run:
+
+./android-build-client build <job_name>
+
+
+Run ./android-build-client --help to see all options.

I tried it, but it does not show me all the available "commands", just the normale options.

It's early in the development stage, but it might be worth to take a look at how lava-tool is handling the commands definition.

There's a lot more code to write to achieve that, but it is a well structured pattern. Just food for thoughts, and something to keep in mind.

=== added file 'utils/cmdline-client/android-build-client'
--- utils/cmdline-client/android-build-client 1970-01-01 00:00:00 +0000
+++ utils/cmdline-client/android-build-client 2013-08-29 17:39:17 +0000
@@ -0,0 +1,199 @@
+#!/usr/bin/env python
+###############################################################################
+# Copyright (c) 2013 Linaro
+# All rights reserved. This program and the accompanying materials
+# are made available under the terms of the Eclipse Public License v1.0
+# which accompanies this distribution, and is available at
+# http://www.eclipse.org/legal/epl-v10.html
+###############################################################################

Why the Eclipse license? Just wondering, since I saw a lot of GPLv3 projects in Linaro.

+def error(msg):
+ print msg
+ sys.exit(1)

Maybe better to redirect it to stderr instead of normal stdout.

+
+def main():
+ global options

Hmmm... why globals here?

+ optparser.add_option("--private", metavar="GROUP",
+ help="Create privaet job accessible to GROUP")

Typo:
s/privaet/private

+
+ options, args = optparser.parse_args(sys.argv[1:])
+ if len(args) < 1:
+ optparser.error("Wrong number of arguments")
+
+ config_dir = os.path.expanduser("~/.config/android-build-client")

It would be better to use the xdg Python module for this instead of relying on hardcoded paths. That module should handle also directory creation.

+ try:
+ os.makedirs(config_dir)
+ except OSError:
+ pass
+ f = open(config_dir + "/cred", "w")
+ print >>f, "%s:%s" % (user, key)
+ f.close()

Why not:

with open( .. "w") as w:
    w.write( ... )