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

Add health-check:health command #44

Closed
wants to merge 18 commits into from

Conversation

idsulik
Copy link

@idsulik idsulik commented Nov 10, 2020

Add command to check helath in console mode - health-check:health.
Made some refactoring

@tylerwoonton tylerwoonton linked an issue Nov 10, 2020 that may be closed by this pull request
@tylerwoonton
Copy link
Contributor

Hi @idsulik,

I can see this is still WIP.

Can I refer you to the linked issue, so you've got a better idea of what we'd like this command to do? #22

Feel free to tag me if you have any questions! 😄

@idsulik
Copy link
Author

idsulik commented Nov 11, 2020

Hi @tylerwoonton , I thought it's ready, but tests fails and I don't understand why it fails only on PHP 7.1
PendingCommand does contains expectsTable method, but fails say it's not...
image

@idsulik
Copy link
Author

idsulik commented Nov 11, 2020

@tylerwoonton , PR is ready) tests code a little messy because of php5 && laravel 5 supports.

@tylerwoonton
Copy link
Contributor

@tylerwoonton , PR is ready) tests code a little messy because of php5 && laravel 5 supports.

@idsulik Excellent. I'll take a look. Thanks!

@tylerwoonton tylerwoonton self-assigned this Nov 11, 2020
Copy link
Contributor

@Gman98ish Gman98ish left a comment

Choose a reason for hiding this comment

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

Cheers for the PR. I think Tyler might have some extra stuff to add, but I've noticed a couple of bits.

Thanks for adding in the docblocks too 👍

tests/Commands/CacheSchedulerRunningTest.php Show resolved Hide resolved
}
}

if (!$isOkay) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion, but instead of keeping track of an isOkay variable, you could check if empty($problems)

Copy link
Author

Choose a reason for hiding this comment

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

Made a little refactoring

Copy link
Contributor

@tylerwoonton tylerwoonton left a comment

Choose a reason for hiding this comment

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

Thanks @idsulik!

Just a couple of tweaks from @Gman98ish and I.

Tag me when you're ready for me to have another look and I'll pull it into an app and have a proper test! 😄


protected $description = 'Check health status';

public function handle()
Copy link
Contributor

Choose a reason for hiding this comment

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

So, one thing I'd like this command to do is allow us to disable certain checks from being ran by this command per run.

If we could add a --disable= option, that'd be great! I imagine we'd pass the $name property of the check and then use that to determine whether to process the check.

https://laravel.com/docs/8.x/artisan#options-with-values

e.g.

php artisan health-check:status --disable=log,http

or

php artisan health-check:status --disable=log --disable=http

Copy link
Author

@idsulik idsulik Nov 11, 2020

Choose a reason for hiding this comment

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

Add --disable=log,cache option

$mock->shouldReceive('name')->andReturn('log');
$mock->shouldReceive('status')->andReturn($status);
}));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests look great! Just leaving a note here to add a test case to cover the --disable option.

Copy link
Author

Choose a reason for hiding this comment

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

Add as separate test method

@idsulik
Copy link
Author

idsulik commented Nov 11, 2020

I think it'll better to add ability to include or exclude specific checks, so I add --only and --except options to status command.
Whay do you think, @tylerwoonton , @Gman98ish ?

@tylerwoonton
Copy link
Contributor

I think it'll better to add ability to include or exclude specific checks, so I add --only and --except options to status command.
Whay do you think, @tylerwoonton , @Gman98ish ?

@idsulik I like that idea! I think by default it should run all the checks and add --only and --except for those use-cases.

@idsulik
Copy link
Author

idsulik commented Nov 11, 2020

I think it'll better to add ability to include or exclude specific checks, so I add --only and --except options to status command.
Whay do you think, @tylerwoonton , @Gman98ish ?

@idsulik I like that idea! I think by default it should run all the checks and add --only and --except for those use-cases.

Yeah, by default it checks all the checks, but you can pass --only OR --except to run/exclude specific checks.

@tylerwoonton
Copy link
Contributor

I think it'll better to add ability to include or exclude specific checks, so I add --only and --except options to status command.
Whay do you think, @tylerwoonton , @Gman98ish ?

@idsulik I like that idea! I think by default it should run all the checks and add --only and --except for those use-cases.

Yeah, by default it checks all the checks, but you can pass --only OR --except to run/exclude specific checks.

That sounds good to me!

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

Successfully merging this pull request may close these issues.

Surface a healthcheck status command
3 participants