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

WIP - fixed #15439 - check db on healthcheck #15441

Closed
wants to merge 2 commits into from

Conversation

snipe
Copy link
Owner

@snipe snipe commented Sep 2, 2024

Not sure this is the right way to approach this, but it's a stab at it anyway. User is saying that the health check should check for database connection as well. I know we kept the health check out of the web middleware specifically so it didn't try to create a buttload of session files, but there might be a clearer approach, perhaps it's own middleware instead of \App\Http\Middleware\CheckForSetup::class.

Tests are failing on index pages with a 401 though, even though it seems to work locally with Postman (which implies to me that the tests might wrong.)

  FAILED  Tests\Feature\AssetModels\Api\IndexAssetModelsTest > viewing asset model index requires authentication
  Expected response status code [201, 301, 302, 303, 307, 308] but received 401.
Failed asserting that false is true.

  at tests/Feature/AssetModels/Api/IndexAssetModelsTest.php:15
     11▕ class IndexAssetModelsTest extends TestCase
     12▕ {
     13▕     public function testViewingAssetModelIndexRequiresAuthentication()
     14▕     {
  ➜  15▕         $this->getJson(route('api.models.index'))->assertRedirect();
     16▕     }
     17▕
     18▕     public function testViewingAssetModelIndexRequiresPermission()
     19▕     {

  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   FAILED  Tests\Feature\Departments\Api\DepartmentsIndexTest > viewing department index requires authentication
  Expected response status code [201, 301, 302, 303, 307, 308] but received 401.
Failed asserting that false is true.

  at tests/Feature/Departments/Api/DepartmentsIndexTest.php:15
     11▕ class DepartmentsIndexTest extends TestCase
     12▕ {
     13▕     public function testViewingDepartmentIndexRequiresAuthentication()
     14▕     {
  ➜  15▕         $this->getJson(route('api.departments.index'))->assertRedirect();
     16▕     }
     17▕
     18▕     public function testViewingDepartmentIndexRequiresPermission()
     19▕     {

  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   FAILED  Tests\Feature\Locations\Api\IndexLocationsTest > viewing location index requires authentication
  Expected response status code [201, 301, 302, 303, 307, 308] but received 401.
Failed asserting that false is true.

  at tests/Feature/Locations/Api/IndexLocationsTest.php:15
     11▕ class IndexLocationsTest extends TestCase
     12▕ {
     13▕     public function testViewingLocationIndexRequiresAuthentication()
     14▕     {
  ➜  15▕         $this->getJson(route('api.locations.index'))->assertRedirect();
     16▕     }
     17▕
     18▕     public function testViewingLocationIndexRequiresPermission()
     19▕     {


  Tests:    3 failed, 10 incomplete, 580 passed (2233 assertions)

Potential (WIP) fix for #15439.

Signed-off-by: snipe <snipe@snipe.net>
@snipe snipe marked this pull request as draft September 2, 2024 19:30
Copy link

what-the-diff bot commented Sep 2, 2024

PR Summary

  • Update in Health Controller
    An improvement is added to the Health Check of our application. Now it includes a check for successful database connections and generates a meaningful response for better understandability of the application's health status.

  • Adjustment in Middleware Assignment
    The setup check which was earlier part of 'web' middleware group has now been moved to the 'api' middleware group. This ensures that any API requests are first validated for application setup before processing further.

  • Enhancement in Route Management
    The '/health' route is now encapsulated in a special group handled by a 'health' middleware. This allows for better control and improved handling of requests directed towards checking the application health status.

Signed-off-by: snipe <snipe@snipe.net>
@snipe snipe closed this Oct 2, 2024
@snipe snipe deleted the check_db_on_healthcheck branch October 2, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant