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

[WIP] Add installer command #11

Open
wants to merge 28 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@sixlive
Collaborator

sixlive commented Aug 20, 2018

Status

WIP

Description

This PR adds an installer command that is able to perform the following actions for both Laravel and Lumen:

  • Optionally publish the configuration file
  • Add the HONEYBADGER_API_KEY with the provided API key to the .env file
  • Add a placeholder HONEYBADGER_API_KEY to the .env.example file
  • Optionally send a test exception to Honeybadger based on the provided API key
  • Some fixes for issues identified by Scrutinizer

Related PRs

n/a

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)
  • Fix Scrutinizer concerns

Steps to Test or Reproduce

> git pull --prune
> git checkout <branch>
> vendor/bin/phpunit
  1. Inside a Laravel or Lumen application install the package
  2. Add the service provider (Lumen or Laravel 5.5 only)
  3. Run php artisan honeybadger:install

Screenshots

screen shot 2018-08-20 at 9 02 10 am

screen shot 2018-08-20 at 8 58 05 am

screen shot 2018-08-20 at 9 07 23 am

@sixlive sixlive self-assigned this Aug 20, 2018

@sixlive sixlive requested a review from joshuap Aug 20, 2018

@joshuap

This comment has been minimized.

Show comment
Hide comment
@joshuap

joshuap Aug 20, 2018

Member

@sixlive this is pretty cool! Here are a few thoughts:

  1. I tried running the command and hitting "enter" for the default values. When it asked for the API key, it let me continue with a blank value, and it said a test exception was sent (even though it must have been rejected by our API). The honeybadger:test command appears to return an error when there is no API key, which is correct.
  2. Not sure if this is possible, but if we could auto-inject the reporting code into app/Exceptions/Handler.php, then this install command would not require any additional steps. Otherwise it's a little awkward to say "run the install command, but then also manually edit this one file". Thoughts on this?
  3. Can we add a welcome message at the end? Here's the welcome message from our Ruby gem (I'll edit one for Laravel below): https://github.com/honeybadger-io/honeybadger-ruby/blob/master/lib/honeybadger/cli/test.rb#L230
⚡ --- Honeybadger is installed! -----------------------------------------------
Good news: You're one deploy away from seeing all of your exceptions in
Honeybadger. For now, we've generated a test exception for you:

    #{notice_url}

If you ever need help:

    - Check out the documentation: https://docs.honeybadger.io/lib/php/index.html
    - Email the 'badgers: support@honeybadger.io

Most people don't realize that Honeybadger is a small, bootstrapped company. We
really couldn't do this without you. Thank you for allowing us to do what we
love: making developers awesome.

Happy 'badgering!

Sincerely,
Ben, Josh and Starr
https://www.honeybadger.io/about/
⚡ --- End --------------------------------------------------------------------

If we can include the notice URL, great. Just remove that part if not.

Member

joshuap commented Aug 20, 2018

@sixlive this is pretty cool! Here are a few thoughts:

  1. I tried running the command and hitting "enter" for the default values. When it asked for the API key, it let me continue with a blank value, and it said a test exception was sent (even though it must have been rejected by our API). The honeybadger:test command appears to return an error when there is no API key, which is correct.
  2. Not sure if this is possible, but if we could auto-inject the reporting code into app/Exceptions/Handler.php, then this install command would not require any additional steps. Otherwise it's a little awkward to say "run the install command, but then also manually edit this one file". Thoughts on this?
  3. Can we add a welcome message at the end? Here's the welcome message from our Ruby gem (I'll edit one for Laravel below): https://github.com/honeybadger-io/honeybadger-ruby/blob/master/lib/honeybadger/cli/test.rb#L230
⚡ --- Honeybadger is installed! -----------------------------------------------
Good news: You're one deploy away from seeing all of your exceptions in
Honeybadger. For now, we've generated a test exception for you:

    #{notice_url}

If you ever need help:

    - Check out the documentation: https://docs.honeybadger.io/lib/php/index.html
    - Email the 'badgers: support@honeybadger.io

Most people don't realize that Honeybadger is a small, bootstrapped company. We
really couldn't do this without you. Thank you for allowing us to do what we
love: making developers awesome.

Happy 'badgering!

Sincerely,
Ben, Josh and Starr
https://www.honeybadger.io/about/
⚡ --- End --------------------------------------------------------------------

If we can include the notice URL, great. Just remove that part if not.

@sixlive

This comment has been minimized.

Show comment
Hide comment
@sixlive

sixlive Aug 20, 2018

Collaborator

Awesome! Thanks for the feedback!

  1. Yeah, it looks like honeybadger-io/honeybadger-php#62 kinda tripped me up here. Will tighten that up.

  2. I'll look into it, I feel like this might be reaching a little far. I'm concerned about adding stuff to a handler of a pre-existing application if someone is doing something pretty custom. I think making this an option in the CLI that they could opt out of would be ideal.

  3. Totally! I think that's great, I've been trying to think of something to add after the output shown above!

Collaborator

sixlive commented Aug 20, 2018

Awesome! Thanks for the feedback!

  1. Yeah, it looks like honeybadger-io/honeybadger-php#62 kinda tripped me up here. Will tighten that up.

  2. I'll look into it, I feel like this might be reaching a little far. I'm concerned about adding stuff to a handler of a pre-existing application if someone is doing something pretty custom. I think making this an option in the CLI that they could opt out of would be ideal.

  3. Totally! I think that's great, I've been trying to think of something to add after the output shown above!

sixlive added some commits Oct 11, 2018

wip
wip
@sixlive

This comment has been minimized.

Show comment
Hide comment
@sixlive

sixlive Oct 11, 2018

Collaborator

@joshuap This is mostly good to go but I wanted to get some feedback on the success messaging.

demo

screenshot

Between the success/fail output of each task and the success message if feels like its a lot of output. I'd like to maybe find a happy medium.

We also need to better define when the success message shows. Which tasks are we willing to let fail and still show the success message?

I also took a stab at breaking up the success message based on whether the test exception was successfully sent to HB or not. Take a look at the SuccessMessageTest class in this PR.

Collaborator

sixlive commented Oct 11, 2018

@joshuap This is mostly good to go but I wanted to get some feedback on the success messaging.

demo

screenshot

Between the success/fail output of each task and the success message if feels like its a lot of output. I'd like to maybe find a happy medium.

We also need to better define when the success message shows. Which tasks are we willing to let fail and still show the success message?

I also took a stab at breaking up the success message based on whether the test exception was successfully sent to HB or not. Take a look at the SuccessMessageTest class in this PR.

@joshuap

This comment has been minimized.

Show comment
Hide comment
@joshuap

joshuap Oct 11, 2018

Member

We also need to better define when the success message shows. Which tasks are we willing to let fail and still show the success message?

If it's possible for .env or .env.example to not exist in some projects, we could let those fail, but then there wouldn't be an API key configured, right? There should always be an error output if the test fails.

The minimum required steps would be 1) add config (with API key), 2) send test.

Between the success/fail output of each task and the success message if feels like its a lot of output. I'd like to maybe find a happy medium.

What are you thinking? Shorter success message? I'm thinking that we don't really need to prompt to send the test error--we always want the test error to be sent, because that's what allows the onboarding process in our UI to continue (it waits for the test error to arrive).

I'm open to any suggestions you have about how you as a user would like the interface/output to work, though.

Member

joshuap commented Oct 11, 2018

We also need to better define when the success message shows. Which tasks are we willing to let fail and still show the success message?

If it's possible for .env or .env.example to not exist in some projects, we could let those fail, but then there wouldn't be an API key configured, right? There should always be an error output if the test fails.

The minimum required steps would be 1) add config (with API key), 2) send test.

Between the success/fail output of each task and the success message if feels like its a lot of output. I'd like to maybe find a happy medium.

What are you thinking? Shorter success message? I'm thinking that we don't really need to prompt to send the test error--we always want the test error to be sent, because that's what allows the onboarding process in our UI to continue (it waits for the test error to arrive).

I'm open to any suggestions you have about how you as a user would like the interface/output to work, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment