----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16306/#review30575 -----------------------------------------------------------
src/main/python/twitter/aurora/client/cli/context.py <https://reviews.apache.org/r/16306/#comment58547> parts = key.split('/') also should probably do a validation here to make sure that len(parts) <= 4 src/main/python/twitter/aurora/client/cli/context.py <https://reviews.apache.org/r/16306/#comment58548> return PartialJobKey(*parts) src/main/python/twitter/aurora/client/cli/context.py <https://reviews.apache.org/r/16306/#comment58549> this would be much more powerful if you could use fnmatch, e.g. us-west/macaw*/test/*staging* rather than just us-west/* this should be pretty easy to do. if any of '*?[]' are in the string then it's not fully bound, instead of just == '*' then you can use fnmatch.filter() to do the matches rather than component == '*' or key.component == component src/main/python/twitter/aurora/client/cli/jobs.py <https://reviews.apache.org/r/16306/#comment58550> not sure if a schema was defined in the designdoc, but any reason not to just use TJSONProtocol.TSimpleJSONProtocol and convert directly from thrift to json here? src/main/python/twitter/aurora/client/cli/jobs.py <https://reviews.apache.org/r/16306/#comment58551> yikes -- a leaky abstraction! we should re-file AURORA-3280 externally. src/main/python/twitter/aurora/client/cli/jobs.py <https://reviews.apache.org/r/16306/#comment58554> snake_case instead of camelCase (even though the thrift is camel.) also, instead of foo_str += '\nbar' foo_str += '\n\tbaz' repeatedly, it's more performant (O(n) vs O(n^2)) to do foo_strs = [] foo_strs.append('bar') foo_strs.append('\tbaz') ... then return '\n'.join(foo_strs) src/main/python/twitter/aurora/client/cli/jobs.py <https://reviews.apache.org/r/16306/#comment58552> indent here is weird src/main/python/twitter/aurora/client/cli/jobs.py <https://reviews.apache.org/r/16306/#comment58555> doubleyikes src/main/python/twitter/aurora/client/cli/jobs.py <https://reviews.apache.org/r/16306/#comment58559> meta-question: does it make sense to put a logger on the context instead of using print? (even if the logger _is_ print?) - Brian Wickman On Dec. 16, 2013, 10:19 p.m., Mark Chu-Carroll wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16306/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2013, 10:19 p.m.) > > > Review request for Aurora, Jonathan Boulle and Brian Wickman. > > > Repository: aurora > > > Description > ------- > > Added an implementation of the "status" verb for jobs. This is the first > command to use the new job wildcards, so there's a bunch of infrastructure > added to the Context to support it. > > This also includes unit tests of the new command. > > > Diffs > ----- > > src/main/python/twitter/aurora/client/api/__init__.py > 60f4011dda0db17ed57d3499637270d466c1b0b8 > src/main/python/twitter/aurora/client/cli/BUILD > aaeede4ac0b1239c1cb9354d15365658e1650ec8 > src/main/python/twitter/aurora/client/cli/__init__.py > 4b771d61464cfa2256d48f0a71ddd5829dd27266 > src/main/python/twitter/aurora/client/cli/context.py > 2ae92ecce8ec3ee2bf66b727e19bc90d1c171eb9 > src/main/python/twitter/aurora/client/cli/jobs.py > 58a723fbc5591565fd51311e0dcfa9567282dbb3 > src/test/python/twitter/aurora/client/cli/BUILD > 89184b649f8c7807a6f599d6f0026902eed11a6c > src/test/python/twitter/aurora/client/cli/test_create.py > da2828f73d32e3d3305fddd4224014a9b847ad2a > src/test/python/twitter/aurora/client/cli/test_kill.py > 8f8095cd2d1dac9d6d2a9891db16cce1d4227eaf > src/test/python/twitter/aurora/client/cli/test_status.py PRE-CREATION > src/test/python/twitter/aurora/client/cli/util.py > 46883ecd696b0f6b691831b65bb5e71367cd652a > > Diff: https://reviews.apache.org/r/16306/diff/ > > > Testing > ------- > > > Thanks, > > Mark Chu-Carroll > >
