Skip to content

Conversation

@atymic
Copy link
Contributor

@atymic atymic commented Jun 5, 2019

As per #170 this add the abilty to configure the guzzle options.

  • Update default config file
  • Update MonitorCollection to make guzzle client with the configured options
  • Added tests to confirm they are respected
  • Updated README to latest config file

@atymic atymic force-pushed the guzzle-configuration branch from 79c0dd0 to 442dd8f Compare June 5, 2019 00:36
$factory->define(Monitor::class, function (Faker\Generator $faker) {
return [
'url' => 'http://localhost:9000',
'url' => sprintf('http://localhost:%d', getenv('TEST_SERVER_PORT')),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When debugging I had to change the server port, and noticed this was hardcoded so I updated it to use the port from the phpunit env.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we set 9000 as the default somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable is set in the phpunit config here:
https://github.com/spatie/laravel-uptime-monitor/blob/master/phpunit.xml.dist

It's currently used here already:
https://github.com/spatie/laravel-uptime-monitor/blob/master/tests/Server.php#L63

We could set a constant as a default somewhere i guess, but since it's already set in the ENV i'm not sure it's needed.

$factory->define(Monitor::class, function (Faker\Generator $faker) {
return [
'url' => 'http://localhost:9000',
'url' => sprintf('http://localhost:%d', getenv('TEST_SERVER_PORT')),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we set 9000 as the default somewhere?


public static function boot()
{
if (empty(getenv(self::ENV_SERVER_PORT))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sixlive
I've added a check here to ensure that the env var is set before booting the server 😄

@atymic atymic force-pushed the guzzle-configuration branch from b32351f to ba10ce2 Compare June 5, 2019 21:00
@atymic
Copy link
Contributor Author

atymic commented Jun 12, 2019

@sixlive
I've added the check above, is this ok?

@atymic
Copy link
Contributor Author

atymic commented Jun 23, 2019

@sixlive
Sorry to bother
Are the changes ok?

@spatie-bot
Copy link

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

@spatie-bot spatie-bot closed this Dec 30, 2019
@atymic
Copy link
Contributor Author

atymic commented Dec 30, 2019

cc @sixlive

@noahfrederick
Copy link

@sixlive Is there anything I can do to help get this feature merged? It looks like @atymic has already addressed all feedback. Thanks.

@atymic
Copy link
Contributor Author

atymic commented Feb 14, 2020

Maybe @freekmurze can help us out here? PR was open for 6 months before autoclosed :(

@freekmurze freekmurze reopened this Feb 14, 2020
@freekmurze
Copy link
Member

Thanks for bringing this to my attention, I'll take care of this.

@atymic could you also update this doc page?
https://github.com/spatie/laravel-uptime-monitor/blob/master/docs/installation-and-setup.md

@atymic
Copy link
Contributor Author

atymic commented Feb 14, 2020

Thanks @freekmurze, i've updated the config in that doc :)

@freekmurze freekmurze merged commit b0b582a into spatie:master Feb 14, 2020
@freekmurze
Copy link
Member

Thanks!

@atymic atymic deleted the guzzle-configuration branch February 14, 2020 08:03
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.

5 participants