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

Implement honcho run PROCESS #71

Closed
wants to merge 7 commits into from

Conversation

chadwhitacre
Copy link
Contributor

This makes honcho run primarily about running a Procfile-specified process in the foreground (the first proposal at #56 (comment)). Previously it was about running an arbitrary shell command, which still works, but now as the fall-through case if a single arg to honcho run doesn't match a Procfile process.

The reason this change is desirable is to be able to use debugging tools such as pdb along with Procfile process aliases (#56). Furthermore, this brings Honcho back into harmony with Foreman's behavior, but in a way that is documented more sensibly (Foreman presents the process case as the exception rather than the command case).

This change is backwards-compatible except for edge cases where a shell command name was used as a Procfile process name. So, for example, if mv was a process name, then calling honcho run mv would now invoke the Procfile process rather than the shell command. Since we also check for the number of arguments when trying to interpret as a process name, this edge case is especially edgy. I think this can safely be considered a backwards-compatible change for all practical purposes.


P.S. For Gittip folks: I'm introducing this fork on Gittip in gratipay/gratipay.com#2384. If/when you change it here's how to revendor it:

  • Go into a local working copy of the fork.
  • echo __version__ = "'$(git rev-parse --short=8 HEAD)'" > honcho/__init__.py
  • python setup.py sdist
  • mv dist/honcho-*.tar.gz ../www.gittip.com/vendor/

Then go back over to www.gittip.com, remove the old version and update requirements.txt.

@chadwhitacre
Copy link
Contributor Author

The tests pass for me against Python 2.7. I'm not set up with tox, so for now I'll watch Travis to see if it runs green for all versions.

@chadwhitacre
Copy link
Contributor Author

Looks like the test failure is in pyflakes. I'll rename the test and force-push to avoid clutter ...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.57%) when pulling 94e01d4 on whit537:run-process into 268d376 on nickstenning:master.

@chadwhitacre
Copy link
Contributor Author

Looking at coverage output on master, it looks like coverage for run is not being reported there either, presumably because of the process boundary during testing. For that reason I'm not going to worry about the tiny dip in coverage reported above for this branch unless you ask me to. :-)

@chadwhitacre
Copy link
Contributor Author

Tests passing! Ready for review, @nickstenning @slafs! :-)

@slafs
Copy link
Collaborator

slafs commented Jun 2, 2014

Yep. This looks good for me since there is no way to specify a command name that has more than one word. Minding the edge cases that Chad pointed out this seems like an acceptable PR. Nevertheless this is a big change in how the run command works so we should probably wait what @nickstenning has to say about this.

@chadwhitacre
Copy link
Contributor Author

Thanks @slafs! Definitely need @nickstenning's blessing, given the vigorous back-and-forth on #56. Hopefully we can reach a consensus! :-)

chadwhitacre added a commit to gratipay/gratipay.com that referenced this pull request Jun 2, 2014
I went with gunicorn for a web server, because it is much simpler to
configure than uwsgi. Note that this Makefile requires a patched version
of Honcho. I made a PR but I don't know if it will be accepted:

nickstenning/honcho#71
chadwhitacre added a commit to gratipay/gratipay.com that referenced this pull request Jun 11, 2014
I went with gunicorn for a web server, because it is much simpler to
configure than uwsgi. Note that this Makefile requires a patched version
of Honcho. I made a PR but I don't know if it will be accepted:

nickstenning/honcho#71
chadwhitacre added a commit to gratipay/gratipay.com that referenced this pull request Jun 13, 2014
I went with gunicorn for a web server, because it is much simpler to
configure than uwsgi. Note that this Makefile requires a patched version
of Honcho. I made a PR but I don't know if it will be accepted:

nickstenning/honcho#71
@galuszkak
Copy link

@nickstenning @slafs any updates on this? :)

@chadwhitacre
Copy link
Contributor Author

Rebased on upstream master.

@galuszkak
Copy link

!m @whit537

@chadwhitacre
Copy link
Contributor Author

Re-did rebase. Now with passing tests! :-)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.65%) when pulling f6ed0d7 on whit537:run-process into 6e9d741 on nickstenning:master.

chadwhitacre added a commit to gratipay/gratipay.com that referenced this pull request Jul 2, 2014
I went with gunicorn for a web server, because it is much simpler to
configure than uwsgi. Note that this Makefile requires a patched version
of Honcho. I made a PR but I don't know if it will be accepted:

nickstenning/honcho#71
chadwhitacre added a commit to gratipay/gratipay.com that referenced this pull request Jul 2, 2014
chadwhitacre added a commit to gratipay/gratipay.com that referenced this pull request Jul 7, 2014
I went with gunicorn for a web server, because it is much simpler to
configure than uwsgi. Note that this Makefile requires a patched version
of Honcho. I made a PR but I don't know if it will be accepted:

nickstenning/honcho#71
chadwhitacre added a commit to gratipay/gratipay.com that referenced this pull request Jul 7, 2014
@chadwhitacre
Copy link
Contributor Author

Caught another edge case (honcho run foo where foo is a one-word command and there is no Procfile).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.96%) when pulling cfcc7eb on gittip:run-process into 6e9d741 on nickstenning:master.

mark-burnett and others added 7 commits July 8, 2014 21:24
Specifying all processes in a Procfile as --quiet, caused `max` to be
called with no arguments, which crashes.
This commit removes all the metaclass magic in honcho.command in favour
of a set of simple nested argparse parsers.
This makes `honcho run` primarily about running a Procfile-specified
process in the foreground. Previously it was about running an arbitrary
shell command, which still works, but now as the fall-through case if a
single arg to `honcho run` doesn't match a Procfile process.

The reason this change is desirable is to be able to use debugging tools
such as pdb along with Procfile process aliases. Furthermore, this
brings honcho back into harmony with Foreman's behavior, but in a way
that is documented more sensibly (Foreman presents the process case as
the exception rather than the command case).

This change is backwards-compatible except for edge cases where a shell
command name was used as a Procfile process name. So, for example, if
`mv` was a process name, then calling `honcho run mv` would now invoke
the Procfile process rather than the shell command. Since we also check
for the number of arguments when trying to interpret as a process name,
this edge case is especially edgy. I think this can safely be considered
a backwards-compatible change for all practical purposes.
@slafs
Copy link
Collaborator

slafs commented Oct 1, 2014

OK. Since there was a big refactoring in honcho lately this PR is out of date and needs some serious work for getting it merged. For now I think it would be best to close it.
@whit537 feel free to comment, rebase or make another PR if you feel like you could tackle this feature again.

@chadwhitacre
Copy link
Contributor Author

Roger that @slafs, ball is in my court to rebase and reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants