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

Output Status of Running Applications #224

Closed
wants to merge 5 commits into from
Closed

Conversation

tubbo
Copy link

@tubbo tubbo commented Feb 6, 2020

Add a puma-dev status sub-command 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 #222

@tubbo tubbo changed the title Output Status of Servers with `docker-dev tatus Output Status of Running Applications Feb 6, 2020
@nateberkopec
Copy link
Member

👍 Needs tests

@nateberkopec
Copy link
Member

👍 That's good enough. Can you resolve the conflicts?

@tubbo
Copy link
Author

tubbo commented Feb 8, 2020

I guess my local master branch was a bit out-of-date, I didn't have a command_test.go, so I rebased and placed my test on top of the existing file. Way easier :)

@nateberkopec
Copy link
Member

👍 Looks like the test you added is failing.

@nonrational
Copy link
Member

You'll also want to add this command to the usage output.

fmt.Fprintf(os.Stderr, "\nAvailable subcommands: link\n")

@tubbo
Copy link
Author

tubbo commented Feb 13, 2020

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 puma-dev server could be down or for some reason not accepting connections over localhost, so for now I'm just printing a message and returning out. But I'm open to suggestions if you folks think it should actually error out here. Just thought it would make tests easier to run if you don't have puma-dev running in the background here.

Copy link
Member

@nonrational nonrational left a 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())
Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

@tubbo tubbo Feb 14, 2020

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.

Copy link
Member

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.

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/main.go Show resolved Hide resolved

func status() error {
client := &http.Client{}
req, err := http.NewRequest("GET", "http://localhost/status", nil)
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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:~$

Copy link
Author

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

Copy link
Member

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

res, err := client.Do(req)

if err != nil {
fmt.Println("puma-dev is not running")
Copy link
Member

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...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally cool with it.

@nonrational nonrational self-requested a review February 25, 2020 04:45
@nonrational
Copy link
Member

@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 main_test.go,TestMainPumaDev(), similar to

t.Run("status", func(t *testing.T) {
statusUrl := fmt.Sprintf("http://localhost:%d/status", *fHTTPPort)
statusHost := "puma-dev"
assert.Equal(t, "{}", getUrlWithHost(t, statusUrl, statusHost))
})

@tubbo
Copy link
Author

tubbo commented Mar 27, 2020

Will do! I'll update the tests. Sorry this took so long.

Tom Scott added 5 commits June 18, 2020 14:33
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
@tubbo
Copy link
Author

tubbo commented Jul 10, 2020

hey @nonrational, is this what you were looking for? I can add some more test cases if you think this isn't sufficient.

wait, something is wrong...I didn't push what I thought I pushed :)

@nonrational
Copy link
Member

@tubbo yeah, looks like you added commits that aren't being reflected in the diff. maybe try an interactive rebase?

@tubbo tubbo closed this Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status Command
3 participants