-
Notifications
You must be signed in to change notification settings - Fork 106
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
Output Status of Running Applications #224
Conversation
👍 Needs tests |
👍 That's good enough. Can you resolve the conflicts? |
I guess my local master branch was a bit out-of-date, I didn't have a |
👍 Looks like the test you added is failing. |
You'll also want to add this command to the usage output. Line 37 in 69b5794
|
Thanks @nonrational, will do! I'm not sure if we should be throwing an error here when the server can't be connected to. I assume this is not an "exceptional" case that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see feedback in-line. I think your approach is perfectly acceptable, but I'd add port override and assertions around the output of the error message.
now that I have the lay of the land, i'm planning to bite off a test that actually spins up puma-dev and symlinks in a dummy rack app so we can test the meat of this thing. given that will need to run on travis, it will almost definitely need to run on a non-standard port. once that's available ( 🤞) it should be trivial to cover the non-error branch of this.
@@ -100,3 +100,7 @@ func TestCommand_link_reassignExistingApp(t *testing.T) { | |||
|
|||
assert.Equal(t, fmt.Sprintf("! App '%s' already exists, pointed at '%s'\n", appAlias, appDir1), actual2) | |||
} | |||
|
|||
func TestCommand_status(t *testing.T) { | |||
assert.Nil(t, status()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make this a bit stronger by utilizing the WithStdOutCaptured
helper in the devtest
package and assert that this simple case prints the correct error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and that it reads the port information correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! I am admittedly quite new to writing Go tests...so I wasn't sure if this was enough coverage over what it does, but I couldn't think of something that would be simple enough to include here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course! admittedly, so am i. but, i've been slowly working on adding enough sugar in here to make testing this stuff a bit easier.
take a look here for an example of how to use that helper i mentioned. i'm happy to answer questions / address feedback.
puma-dev/cmd/puma-dev/command_test.go
Lines 85 to 92 in 69b5794
StubFlagArgs([]string{"link", "-n", appAlias, appDir1}) | |
actual1 := WithStdoutCaptured(func() { | |
if err := command(); err != nil { | |
assert.Fail(t, err.Error()) | |
} | |
}) | |
assert.Equal(t, fmt.Sprintf("+ App '%s' created, linked to '%s'\n", appAlias, appDir1), actual1) |
cmd/puma-dev/command.go
Outdated
|
||
func status() error { | ||
client := &http.Client{} | ||
req, err := http.NewRequest("GET", "http://localhost/status", nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does puma-dev always expose status on port 80? if you're running on a nonstandard port, you'd need to access it elsewhere.
admittedly, :80 is very likely the most common case. but, it might be worth respecting fHTTPPort
when checking the status so puma-dev -http-port 8080 status
would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, but the status API documentation makes no mention of needing to use a different port other than :80
here. You can just run curl -H "Host: puma-dev" localhost/status
, and as long as localhost
resolves to puma-dev
's DNS server, it should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could consider the dynamic port a future enhancement. i think this will get the job done for the majority of users.
eventually, we'll need to have dynamic port in order to call this feature complete. if, say, you're not interested in running puma-dev as a daemon on 80, you won't be able to use status
but maybe that's okay, as long as the error message communicates that.
Here's a quick example of checking status of two different puma-dev's running.
osiris:~$ curl -H "Host: puma-dev" localhost/status # get status of puma-dev running as daemon
{}
osiris:~$ curl -H "Host: puma-dev" localhost:9280/status # get status of puma-dev running interactively
{}
osiris:~$ curl -H "Host: epimetheus" localhost:9280/ | head -n1 # boot an app via interactive puma-dev
<!DOCTYPE html>
osiris:~$ curl -H "Host: puma-dev" localhost:9280/status | head -n1 # get status of puma-dev running interactively, now that an app has been booted
{"epimetheus":{"scheme":".......}
osiris:~$ curl -H "Host: puma-dev" localhost/status # get status of puma-dev running as daemon
{}
osiris:~$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting...are you running on macOS or Linux? I'm getting a different response when I try to curl
the status using the default HTTP port number on macOS Catalina:
$ curl -H "Host: puma-dev" localhost:9280/status
curl: (7) Failed to connect to localhost port 9280: Connection refused
Some other info of note:
$ launchctl list | grep io.puma
27687 0 io.puma.dev
$ ps -ef | grep puma-dev
502 27687 1 0 11:17AM ?? 0:00.36 /usr/local/Cellar/puma-dev/0.12/bin/puma-dev -launchd -dir ~/.puma-dev -d test -timeout 15m0s
502 77111 76171 0 12:21PM ttys005 0:00.47 vim cmd/puma-dev/command.go
502 77879 76171 0 12:22PM ttys005 0:00.00 grep puma-dev
Also, of course:
$ curl -H "Host: puma-dev" localhost/status
{}
This may be due to a file (which was apparently generated by puma-dev at some point) in /etc/resolver/localhost:
# Generated by puma-dev
nameserver 127.0.0.1
port 9253
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm sorry. i didn't include enough context. i'm explicitly running puma-dev in interactive mode alongside my daemonized puma-dev on macOS in order to prove that /status
listens on whatever port puma-dev is configured to use.
osiris:puma-dev(cover-homedir-report-coverage)$ puma-dev
* Directory for apps: /Users/norton/.puma-dev
* Domains: test
* DNS Server port: 9253
* HTTP Server port: 9280
* HTTPS Server port: 9283
! Puma dev listening on http and https
! Booting app 'epimetheus' on socket /Users/norton/.puma-dev/epimetheus/tmp/puma-dev-63200.sock
epimetheus[63609]: bash: no job control in this shell
epimetheus[63609]: Puma starting in single mode...
epimetheus[63609]: * Version 4.3.0 (ruby 2.6.5-p114), codename: Mysterious Traveller
epimetheus[63609]: * Min threads: 0, max threads: 5
epimetheus[63609]: * Environment: development
epimetheus[63609]: * Listening on unix:/Users/norton/.puma-dev/epimetheus/tmp/puma-dev-63200.sock
epimetheus[63609]: Use Ctrl-C to stop
! App 'epimetheus' booted
! Killing 'epimetheus' (63609)
epimetheus[63609]: - Gracefully stopping, waiting for requests to finish
epimetheus[63609]: === puma shutdown: 2020-02-14 11:41:54 -0500 ===
epimetheus[63609]: - Goodbye!
! Killing 'epimetheus' (63609)
* App 'epimetheus' shutdown and cleaned up
cmd/puma-dev/command.go
Outdated
res, err := client.Do(req) | ||
|
||
if err != nil { | ||
fmt.Println("puma-dev is not running") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure if we have enough information to say for sure that puma dev isn't running. what we do know is that we weren't able to get the status from GET localhost:80/status
.
by way of heading off future new issues saying "puma-dev is running but status says it's not!!1", how do you feel about fmt.Sprintf("Unable to lookup puma-dev status. Is puma-dev listening on port %s?", fHTTPPort)
?
edit: not sure what the capitalization rules are. I guess it should be sentence case. Updating casing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally cool with it.
@tubbo still interested in getting this over the finish line? i think the testing story is now a little more straightforward. you should be able to call your status method on a running puma-dev inside puma-dev/cmd/puma-dev/main_test.go Lines 133 to 138 in 7312d73
|
Will do! I'll update the tests. Sorry this took so long. |
Add a `puma-dev status` subcommand to output the status of all applications currently running through `puma-dev`. Uses the Status API requested from `http://localhost/status` with `Host: "puma-dev"`. Fixes puma#222
wait, something is wrong...I didn't push what I thought I pushed :) |
@tubbo yeah, looks like you added commits that aren't being reflected in the diff. maybe try an interactive rebase? |
Add a
puma-dev status
sub-command to output the status of allapplications currently running through
puma-dev
. Uses the Status APIrequested from
http://localhost/status
withHost: "puma-dev"
.Fixes #222