Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support dryrun behavior when executing shell commands #324

Closed
hindman opened this issue Feb 6, 2016 · 12 comments

Comments

@hindman
Copy link

commented Feb 6, 2016

Almost every time I write a script to execute shell commands, I end up with a function that looks roughly like this:

def run(cmd, dryrun=False):
    if dryrun:
        # Just print cmd or maybe return cmd with some other data.
    else:
        # Actually run it with subprocess, typically returning stdout, exitcode, cmd, etc

This sort of thing is very handy for development work and debugging. It is also a useful thing to have if you want to write unit tests for your code.

Our team uses Fabric a lot and I created a module that wraps Fabric in a class that supports dryrun, but this effort was fairly awkward and requires a certain amount of carefulness from users of the class.

Are there plans to include dryrun behavior in Invoke and ultimately in Fabric 2.0? I don't see anything in the docs or invoke codebase so far. I'd be willing to lend a hand, but wanted to get a reality check first.

Thanks.

@thebjorn

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2016

Sounds useful. I put together https://github.com/pyinvoke/invoke/compare/master...thebjorn:dryrun?expand=1 to see how feasible it would be. Beware, it's my first time inside this code base, so I might have overlooked important issues. I can work it into an acceptable PR if the feature is wanted and I'm on the right track..

@bitprophet

This comment has been minimized.

Copy link
Member

commented Feb 8, 2016

I could swear we had a ticket for this but I can't find one. Could just be there's a ticket in Fabric's tracker I marked as '2.0' that I haven't ported here yet (edit: yea, fabric/fabric#26 - and that's not marked invoke or 2.0, heh.)

This would definitely be an Invoke-level feature, not a Fabric 2.0 one. @thebjorn's approach is close to what I would have defaulted to - thanks!

The main problem with dry-run in general is most nontrivial user-level logic (or 'branchy' helpers like def exists(path): return run('test -f {0}'.format(path)).ok) gets thrown off by a generic (and positive/exited-0) fake result. I.e. one size never really fits all and you won't be able to reliably test complex workflows in a satisfactory manner. (E.g. Chef runs have this same problem, or did when I last used it.)

However, an 80% solution is better than a 0% one and we can always try to enhance it later (e.g. providing some sort of "hey, when run sees XYZ command, make it dry-return instead of the default" mechanism, similar to the new prompts stuff). So I'm down with implementing something along @thebjorn's lines.

When I poke at the next 'bundle o features' milestone I'll put something together working off of that.

@bitprophet bitprophet added this to the 0.13 milestone Feb 8, 2016

@hindman

This comment has been minimized.

Copy link
Author

commented Feb 8, 2016

Sounds good. Thanks for considering this.

Regarding user-level logic, in the Fabric-wrapping class that our team uses, all tasks are executing in a context where the user-level code knows whether we are in dryrun mode. It's then up to the user-level code to handle that mode (eg, returning hard-coded data suitable for the task at handle). It's worked reasonably well for our team -- lots of complex Fabric tasks, with plenty of branching logic, but having good unit test coverage via the data generated during dryrun mode. It was somewhat awkward to staple that sort of behavior on top of Fabric, so it the idea were baked nicely into the Invoke layer that would be a great feature.

@bitprophet

This comment has been minimized.

Copy link
Member

commented Feb 8, 2016

@hindman Yea, for non-contextualized mode that'd be tricky, but if you're using contexts (which will probably be the default/only mode come 1.0 / fab 2.0) that would probably line up nicely; inform the top levels of the code that config['run']['dry'] = True, your own code would be able to inspect that state, and anytime execution actually trickles down into a run call, the empty/positive default is returned.

@chaimpeck

This comment has been minimized.

Copy link

commented Apr 28, 2016

I came up with a very primitive version of the "dry run" that works for me. Perhaps this idea could be expanded upon to be more tightly integrated with fabric.

I made a task that, if invoked, will overload the run and local functions.

@task
def dryrun():
    """Show, but don't run fabric commands"""

    global local, run

    # Redefine the local and run functions to simply output the command
    def local(command, capture=False, shell=None):
        print "(dryrun)[localhost] %s" % (command)

    def run(command, shell=True, pty=True, combine_stderr=None, quiet=False,
            warn_only=False, stdout=None, stderr=None, timeout=None, shell_escape=None,
            capture_buffer_size=None):
        print "(dryrun)[%s] %s" % (env.host_string, command)

I put that in my fabfile.

Now, if I hypothetically have the two following tasks:

@task
def test1():
    local("ls")

@task
@hosts('my.host.net')
def test2():
    run("ls")

I can execute them "dry" as follows:

$ fab dryrun test1
(dryrun)[localhost] ls

Done.
$ fab dryrun test2
[my.host.net] Executing task 'test2'
(dryrun)[my.host.net] ls

Done.

This way, I can view the commands that are going to be executed before they are actually executed.

Obviously, anything that is relying on capture=True is going to fail, and there are many other reasons why this is not a complete solution. But, this might be helpful to somebody who finds this thread, and like I said, maybe it can be expanded into something more complete.

@stonefury

This comment has been minimized.

Copy link

commented Apr 29, 2016

I couldn't get that working with the decorator. I just defined the dryrun function and executed fab like this:

fab -u ubuntu dryrun deploy:branch=branch_release_2.11,host=10.0.1.10

@chaimpeck

This comment has been minimized.

Copy link

commented May 2, 2016

If you don't use the @task decorator, fabric will automatically consider all functions that do not begin with an underscore as a task. If you do want to use the decorator, you need to import it.

Either way, I'm glad that you found it useful.

@bitprophet bitprophet modified the milestones: 0.13, 0.14 Jun 11, 2016

@brutus

This comment has been minimized.

Copy link

commented May 17, 2017

I just needed this function in a project and ended up with something real quick, based on thebjorn's approach:

https://github.com/pyinvoke/invoke/compare/master...brutus:dryrun?expand=1

I also added a ignore_dry argument for the run settings, to, well, ignore the dry settings (e.g. for a given run call). As I see the dry settings not as a "security setting", I think that might be okay.

I often use simple run commands (e.g. test for file existence, get paths, etc.) to build up bigger ones. I then want to be able to view those big ones (without actually running them) and don't care for all the small steps that lead up to them.

I haven't had a good look at invoke's code base in quiet some time (sweet to see so much nice changes btw), so I don't know, if I used the correct "internal structures" and stuff… might be a somewhat ugly hack.

But anyway: it works for me so far and the tests run fine. I used this collection for testing. Shame on me tho, I didn't write any new test, sorry :( But as another deadline is looming at the end of the month, it might be a while before I use this again.

I'm interested in the status of the issue and what you think though.

@hindman

This comment has been minimized.

Copy link
Author

commented Jul 15, 2017

@bitprophet Any status update on this issue? I'm willing to help out, if it would help, but could use some general guidance on where things stand and whether you have thoughts on the best way to implement this.

@bitprophet bitprophet modified the milestones: 0.21, p3 Jul 12, 2018

@bitprophet

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Poking at trialing a MVP version of this now for work reasons. I still like @thebjorn's approach (more below) and it applies mostly cleanly to modern Invoke, with one minor issue: -D got taken by --list-depth in the interim. Brainstorming:

  • No shortflag. Not ideal esp given this is likely to be frequently used.
  • -d and -r (other easy mnemonics for Dryrun / dRyrun) already taken
  • -y (drYrun) not taken but I'd like to reserve it for some equivalent to the common Unix-ism of "yes to all prompts"
  • -f (for 'fake') already taken
  • maybe -R (again, dRyrun, just capitalized)?
  • alternately, -t for "Test"? (may even want the long flag to be --test? but "dry run" is very common parlance and "test" frequently means other things...)

I think -R wins so gonna go with that.


Re: @brutus' addition of --ignore-dry: I think I see the need for this to a degree but I suspect it's "more correctly" handled by some sort of 'shared per-task args' setup (this is a longstanding feature need, I don't know the ticket number offhand) so one could do something like inv something --dry otherthing --dry thirdthing.

Also I'm not actually super clear on why @brutus' example code (with its "two copies of each task, one with dry baked in, one without") needs an ignore_dry, because I am relatively sure that explicitly giving dry=False in a run() call overrides a CLI-level --dry (or a config level dry=True or etc). The kwargs for run are supposed to only default to the greater context's value if they are still None (i.e. the function level default value).


Re: overall implementations past this point, there's a lot we could do but as said I think a minimum viable product approach works best here, we can make a new ticket for fancier iterations.

My plan is to try out this basic approach in my real-world needs, add any other obvious easy wins, and put in tests/etc.

bitprophet added a commit that referenced this issue Jul 1, 2019

Get patch for #324 working enough for runtime again
- needed to add 'dry' to default config layout
- needed to change CLI shortflag to not conflict w/ newer arg

bitprophet added a commit that referenced this issue Jul 1, 2019

bitprophet added a commit that referenced this issue Aug 6, 2019

Get patch for #324 working enough for runtime again
- needed to add 'dry' to default config layout
- needed to change CLI shortflag to not conflict w/ newer arg

bitprophet added a commit that referenced this issue Aug 6, 2019

bitprophet added a commit that referenced this issue Aug 6, 2019

@bitprophet

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Tidied this up a bit more, wrote changelog and tests, and did a quick test to prove to myself it works as-is w/o modifications in Fabric 2 (the formula works! sometimes!). E.g. with master invoke installed, fab -H localhost --dry-run -- anonymous command line here echoes-and-returns as expected.

@bitprophet

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Merged.

@bitprophet bitprophet closed this Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.