Fetaure/slug group routing

[albgau]

Hi, For my usage i need to use grouping check but unfortunately without GET parameter in url. So i purpose my adds where grouping check can be use by a slug in route and too by GET method.

I updated the doc page with the new usage description like this example :

The following URLs accept an optional query parameter ?group= to specify the check group:

/monitor/health/checks?group=liveness
/monitor/health/http_status_checks?group=liveness
/monitor/health/http_status_check/check_id?group=liveness
/monitor/health/run?group=liveness
/monitor/health/run/check_id?group=liveness

Or use slug URL /group/{group} to specify the check group:

/monitor/health/checks/group/liveness
/monitor/health/http_status_checks/group/liveness
/monitor/health/http_status_check/check_id/group/liveness
/monitor/health/run/group/liveness
/monitor/health/run/check_id/group/liveness

Thanks for your job.

[albgau]

hi @kbond, i saw you are main reviewer on pull request,can you check my PR please.

[kbond]

Thanks for the ping @albgau, I missed this.

I'm not in favor of this but it's because I want to deprecate and eventually remove the controller (and js/css) entirely (or at least the concrete implementation). I don't think the frontend stuff belongs in this bundle.

It's pretty trivial to create your own health check controller that would fit your site's theme/needs exactly (this is what I do).

[albgau]

ok, Will you keep the cli-command ? And the nagios integration will left too if you remove controller parts ?

More precisely, we will think use nagios's call to url liipmonitorBundle in order to do check the health of app but URL are protected by cleartrust and cleartrust is not nice with GET's url.In a second time call to liipmonitor will be use for probe liveness, startup and readiness in kubernetes. To do this I suppose the cli-command must be enough or have-you another suggestion ?

For our use js/css was not important, only the http_code and json response was.

[kbond]

Will you keep the cli-command ?

Yes.

And the nagios integration will left too if you remove controller parts ?

You're right. I think it makes sense to have a NagiosController.

For our use js/css was not important, only the http_code and json response was.

Removing the js/css/template is the main thing I want to remove. I'm not opposed to keeping an ApiController that returns json. Or even adding controllers for specific monitoring services (that work via return status codes or json/xml).

More precisely, we will think use nagios's call to url liipmonitorBundle in order

So to clarify, this PR is to help with your app's nagio's implementation?

[albgau]

For our use js/css was not important, only the http_code and json response was.

Removing the js/css/template is the main thing I want to remove. I'm not opposed to keeping an ApiController that returns json. Or even adding controllers for specific monitoring services (that work via return status codes or json/xml).

What do you think of return in the same time the status codes in the header and json/xml ?

More precisely, we will think use nagios's call to url liipmonitorBundle in order

So to clarify, this PR is to help with your app's nagio's implementation?

Yes,that's it i need nagios call http page without get parameter,that's why i add route and tweak controller to work with slug group.

[kbond]

What do you think of return in the same time the status codes in the header and json/xml ?

Can you explain this a bit more? I'm not sure what you mean.

Yes,that's it i need nagios call http page without get parameter,that's why i add route and tweak controller to work with slug group.

I don't know anything about nagios but a standard nagios setup does not require this, correct? It is required for you because of "cleartrust" that you are using in your setup?

[albgau]

Can you explain this a bit more? I'm not sure what you mean.

Actually in the case of a bad check, when you perform a health check with "/run" the return is a json with global status at "ko"the http status code in the header is 200/ok, And if you perform the same check by "/http_status_checks" the return is 502 in the header. So the idea will be to return the json details and in the header the 502 code instead of 200.

I don't know anything about nagios but a standard nagios setup does not require this, correct? It is required for you because of "cleartrust" that you are using in your setup?

Exactly Cleartrust protects access by sso authentication.And in order to make health check without authentification, in Cleraturst we exclude some url/file extension. So I named the group of check with extension ".check" by example liveness.check and in cleartrust we exclude all *.check.And url like URL?group=liveness cannot be exclude and use for our health check interrogations.

[kbond]

Actually in the case of a bad check, when you perform a health check with "/run" the return is a json with global status at "ko"the http status code in the header is 200/ok

Understood, I certainly believe the status code should be 502 in this case. I am hesitant to change this for a minor version though. I am really not familiar with how the js works and could be a BC break. As for putting the status code in the header, I'll still suggest you create your own controller to do exactly what you'd like.

Exactly Cleartrust protects access by sso authentication.

I do feel like this is a specific use-case for your app. Again, I think a custom controller is your best bet. You wouldn't need to flood your app with routes that aren't required.

This is the entirety of my custom controller:

public function healthAction(Runner $runner): Response
{
    $runner->addReporter($reporter = new ArrayReporter());
    $runner->run();

    return $this->render('admin/health.html.twig', ['results' => $reporter->getResults()]);
}

You'd probably have some more logic as you're using groups but it would still be small.

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This pull request has been automatically closed. Feel free to re-create if it is still relevant.