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

Update php fpm check to allow ping reply to be configured. #1582

Closed
wants to merge 2 commits into from
Closed

Update php fpm check to allow ping reply to be configured. #1582

wants to merge 2 commits into from

Conversation

squaresurf
Copy link
Contributor

No description provided.

@LeoCavaille
Copy link
Member

Cool thanks @squaresurf for your contribution.
Just a suggestion: could you add something in the integrations tests to test this new option, for instance set it to blah and test that the can_ping service check returns effectively a CRITICAL status?

@LeoCavaille LeoCavaille self-assigned this Apr 29, 2015
@squaresurf
Copy link
Contributor Author

I've added a test for a bad ping reply. @LeoCavaille would you take a look and make sure I did it correctly? I've not used this test framework before.

@LeoCavaille
Copy link
Member

LGTM thanks again 👍

@squaresurf
Copy link
Contributor Author

@LeoCavaille do you know why the test fails? It doesn't seem to be related to my PR.

@LeoCavaille
Copy link
Member

No the PHP-FPM test succeeds which is what matters here. We're currently fixing the CI @degemer is working on it, basically we need to have a different setup for contributors and external forks because they can't use our S3 cache due to credentials issues.
Closing this PR, merged upstream your changes in a rebased commit 552b5d1.
Thanks.

@squaresurf
Copy link
Contributor Author

Thank you!

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 62d75b6 on squaresurf:php-fpm-ping-reply into ** on DataDog:master**.

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

Successfully merging this pull request may close these issues.

3 participants