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

Return correct status codes in process-check-result API #9354

Conversation

DamianoChini
Copy link
Contributor

@DamianoChini DamianoChini commented Apr 21, 2022

Hi all!

This PR addresses the issue #9353.

fixes #9353

In the implementation we modified the ProcessCheckResult function in order to return an enum that describes the actual result of the function. In the ApiActions::ProcessCheckResult function we check the returned enum and create a result with an appropriate code and status message.
Please let us know if you think other status codes can be more appropriate.

Below you find some curl request with which we can test the changes, in particular to check that the API returns an appropriate reply when we send a process-check-result with an older timestamp with respect to the last check result of the host.

Tests

Initial process-check-result:

curl -v -k -u root:<pwd> 'https://localhost:5665/v1/actions/process-check-result' -H 'accept: application/json' -XPOST -d '{"type": "Host", "host": "my-host", "exit_status": 1, "plugin_output": "initial pcr", "execution_start": "1650553018.000000", "execution_end": "1650553018.000000"}'
should return
{"results":[{"code":200.0,"status":"Successfully processed check result for object 'my-host'."}]}

Process-check-result with older execution_start:

curl -v -k -u root:<pwd> 'https://localhost:5665/v1/actions/process-check-result' -H 'accept: application/json' -XPOST -d '{"type": "Host", "host": "my-host", "exit_status": 1, "plugin_output": "pcr with older timestamp", "execution_start": "1650553017.000000", "execution_end": "1650553017.000000"}'
should return:
{"results":[{"code":409.0,"status":"Newer check result already present. Check result for 'my-host' was discarded."}]}

Process-check-result with newer execution_start:

curl -v -k -u root:<pwd> 'https://localhost:5665/v1/actions/process-check-result' -H 'accept: application/json' -XPOST -d '{"type": "Host", "host": "my-host", "exit_status": 1, "plugin_output": "pcr with newer timestamp", "execution_start": "1650553019.000000", "execution_end": "1650553019.000000"}'
should return:
{"results":[{"code":200.0,"status":"Successfully processed check result for object 'my-host'."}]}

@cla-bot cla-bot bot added the cla/signed label Apr 21, 2022
@DamianoChini DamianoChini force-pushed the feature/return-correct-status-in-process-check-result-api branch 2 times, most recently from 849ea23 to de30ed5 Compare April 21, 2022 15:18
@julianbrost julianbrost self-requested a review April 22, 2022 10:33
lib/icinga/apiactions.cpp Outdated Show resolved Hide resolved
lib/icinga/checkable.hpp Outdated Show resolved Hide resolved
lib/icinga/apiactions.cpp Outdated Show resolved Hide resolved
@julianbrost julianbrost added the area/api REST API label Apr 22, 2022
@DamianoChini DamianoChini force-pushed the feature/return-correct-status-in-process-check-result-api branch 2 times, most recently from 70184b9 to 033bd4f Compare April 22, 2022 14:03
@DamianoChini
Copy link
Contributor Author

Hi @julianbrost !
Thank you very much for the fast reply, I pushed the changes as per you feedback.
Sorry for the multiple pushes to the branch, I keep forgetting setting my IDE to use tabs instead of whitespaces for indentation :)

@julianbrost julianbrost self-requested a review April 25, 2022 07:55
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

I didn't consider how long the names would get when moving the enum into the class. I suggest adding using Result = Checkable::ProcessingResult; to ApiActions::ProcessCheckResult() and Checkable::ProcessCheckResult() and then using that alias to keep things a bit shorter.

lib/icinga/checkable.hpp Outdated Show resolved Hide resolved
@DamianoChini DamianoChini force-pushed the feature/return-correct-status-in-process-check-result-api branch from 033bd4f to f0e5d9c Compare April 26, 2022 06:41
@julianbrost julianbrost self-requested a review April 26, 2022 07:51
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Just one tiny indentation mishap left to fix, then I'm happy with the PR.

lib/icinga/checkable-check.cpp Outdated Show resolved Hide resolved
@DamianoChini DamianoChini force-pushed the feature/return-correct-status-in-process-check-result-api branch from f0e5d9c to 9d9810b Compare April 26, 2022 11:34
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

A little bit over-engineered, but better than the other way around.

@julianbrost Also see my OP edit.

@Al2Klimov Al2Klimov requested a review from julianbrost May 5, 2022 13:14
@julianbrost julianbrost merged commit 4184dcd into Icinga:master May 5, 2022
@julianbrost julianbrost added this to the 2.14.0 milestone Jun 10, 2022
yhabteab pushed a commit that referenced this pull request Sep 5, 2022
…atus-in-process-check-result-api

Return correct status codes in process-check-result API
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.

Process-check-result API should return whether the process-check-result was processed or discarded
3 participants