Code review comment for lp://qastaging/~pfalcon/linaro-android-build-tools/cmdline-client
Revision history for this message
| Paul Sokolovsky (pfalcon) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
demo site
(Get the code!)
Hello Milo,
Thanks for comments!
On Fri, 30 Aug 2013 14:31:38 -0000
Milo Casagrande <email address hidden> wrote:
> Review: Needs Fixing
>
> Hey Paul,
>
> following my comments, there's some typos that need fixing and a
> couple of suggestion/
> cleaned the typos it should be ready to go.
>
> +Basic android-
> +======
> +
> +This tool provides proof-of-concept command-line client for Linaro
> Android +Build Jenkins master (android-
> 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
fixed
>
> +useful, please share you comments and suggestion using this
> bugtracker:
>
> s/you comments/your comments
fixed
>
> +
> +https:/
> +
> +to help its improvement and evolution.
> +
> +
> +Quick start
> +-----------
> +
> +1. You should have access to Jenkins at:
> +
> +http://
> +
> +(Generally available for Linaro Android team members).
> +
> +2. Download android-
> only
>
> s/to depend/it depends
You probably mean replacing the whole starting phrase? Nope, that
loses meaning. It not just randomly depends only on stdlib, it's
purposedly written to ;-).
>
> +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-
> +
> +3. Look up your Jenkins API token by visiting
> +https:/
> +"Show API Token..." button.
> +
> +4. Run:
> +
> +./android-
> +
> +Enter you Jenkins username and API token. Note that this will cache
> these
>
> s/you Jenkins/your Jenkins
fixed
>
> +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:/
> for more +info) in a file. Run:
> +
> +./android-
> +
> +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
Nope, again changes meaning ;-). "job private for particular group" ==
"job which is private for particular group"
>
> +
> +6. To schedule a build, run:
> +
> +./android-
> +
> +
> +Run ./android-
>
> 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.
Yeah, that's too complicated creating "perfect" UI may take more effort
than functionality in a simple tool. But one may look inside
readable Py code after all and learn much more. I just changed --help
to:
Usage: android-
>
>
> === added file 'utils/
> --- utils/cmdline-
> +0000 +++ utils/cmdline-
> 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://
> +######
>
> Why the Eclipse license? Just wondering, since I saw a lot of GPLv3
> projects in Linaro.
Existing code in linaro-
just followed existing pattern. It worries me too, especially because
Eclipse license is not compatible with GPL, but making a license zoo of
one project without good consideration is hardly good idea.
>
> +def error(msg):
> + print msg
> + sys.exit(1)
>
> Maybe better to redirect it to stderr instead of normal stdout.
fixed
>
> +
> +def main():
> + global options
>
> Hmmm... why globals here?
options are passed via global var
>
> + optparser.
> + help="Create privaet job accessible to GROUP")
>
> Typo:
> s/privaet/private
fixed
>
> +
> + options, args = optparser.
> + if len(args) < 1:
> + optparser.
> +
> + config_dir = os.path.
>
> 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.
Not part of standard library. Implements "freedesktop.org standards". I
don't try to implement freedesktop.org standards, just do sensible
things in KISS way ;-).
>
> + try:
> + os.makedirs(
> + 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( ... )
I even sometimes use that nowadays, but still think it is *optional*,
*extra* layer of syntactic sugar (i.e. its availability in language
doesn't mean it has to be used all the time, and it still may be
confusing for people who didn't know Common Lisp ;-) ).
--
Best Regards,
Paul
Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://
http://