Details

Reviewers

thcipriani
dduvall
demon

Group Reviewers

Release-Engineering-Team

Patch without arc

git checkout -b D6 && curl -L https://phabricator.wikimedia.org/D6?download=true | git apply

Summary

interactive shell built on prompt_toolkit and tmuxp

Test Plan

run bin/iscap

Diff Detail

Repository

rMSCA Scap

Branch

iscap-squash

Lint

Lint Warnings

Unit

No Test Coverage

Build Status

Buildable 112
Build 112: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Gave some basic review here, started to go more in depth, but ran out of day. One weird thing that may be a prompt-tool kit bug: options only autocompleted for me on the first run so: typing Deploy shows completions for -s or -l, etc. then I ran: Deploy -l scap*01, then typing: Deploy again doesn't show any of the completions.

scap/main.py
916	keep. config_deploy stuff.
932	keep. config deploy stuff.
947	part of config_deploy patch too. If the config option is False, we don't want to pass it since python 'cause "truthy"
scap/monitor.py
1	Maybe break this file into a separate diff.
38	FWIW, you could use the requests library here.
48	probably ought to use some kind of logging
scap/prompt/context.py
3	imported but unused
9	Imported but unused
113	unintuitive method name, might be a good use of an `@property`. Interface would then be: `context.cmd = 'cd /home/thcipriani'; context.cmd.run()`
116	doesn't provide any shell expansions. `cd ~` or `cd dep*` etc.
136	probably should use a more explicit exception
179	no need for an `@property` and `@cwd.setter` if you're just setting `self._cwd`, just change the variable name to `self.cwd` and you should be able to get it and set it.
281	probably should use a more explicit exception
319	probably should use a more explicit exception
scap/ssh.py
161	currently `self._run_with_reporter` uses `cluster_ssh` and if no reporter is attached then `cluster_ssh_threaded` is run. This difference in behavior is non-obvious based on the fact that the decision which of the cluster methods to run is based on calling `.progress()` on an and instance of the Job class.
276	This should utilize a logger, particularly since `deploy-local` now returns json.
scap/tasks.py
98	log_context decorator requires logger argument
175	log_context decorator requires logger argument
251	log_context decorator requires logger argument
329	Needs to be removed if you're removing the logger argument, log_context decorator requires logger argument
406	requires `logger` keyword argument. Also, `log_context` is removed elsewhere.
452	log_context decorator requires logger argument
614	this code seems to be in a comment.
setup.py
25	also needed: jinja2 pyaml requests

In D6#326, @thcipriani wrote:

Gave some basic review here, started to go more in depth, but ran out of day. One weird thing that may be a prompt-tool kit bug: options only autocompleted for me on the first run so: typing Deploy shows completions for -s or -l, etc. then I ran: Deploy -l scap*01, then typing: Deploy again doesn't show any of the completions.

That's probably a bug in my completer class, I'll look into that.