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

First pass at allowing custom checkers. #12

Closed

Conversation

kezabelle
Copy link
Contributor

Would fulfil #3

Should, in theory, support Django 1.4.2 (introduced the vendored six library) through to 1.7+

Usage in settings is:

WATCHMAN_CHECKS = (
    'module.path.to.callable',
    'another.module.path.to.callable',
)

Checks now have the same contract as context processors: they consume a request and return a dict whose keys are applied to the JSON response:

def my_checker(request):
    return {'x': 1}

In the absence of any checkers, a 404 is thrown, which is then handled by the json_view decorator.

Only a subset of checks may be run, by passing only_check=module.path.to.callable&only_check=... in the request URL. Only the callables given in querystring, which are in the WATCHMAN_CHECKERS should be run, eg:

curl -XGET http://127.0.0.1:8080/_is_app_responding/?only_check=watchman.views.caches_status

@mwarkentin
Copy link
Owner

Hi @kezabelle , just wanted to say thanks for submitting this. I'm hoping to be able to review later today.

@@ -1,5 +1,9 @@
from django.conf import settings


# TODO: these should not be module level.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain what you mean here? Maybe open a separate issue for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, that was a note to myself precisely to open it as a separate issue later, and wasn't intended to be committed as is.

The side-effect of doing it this way (and I'm guilty of it myself; a lot) is that it is cached at the module-level. It mostly isn't a problem for the app as-is because the biggest use-case it may break is override_settings, which isn't being used.

There was a ticket about documenting a specific way, but no resolution came of it.

Copy link
Owner

Choose a reason for hiding this comment

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

Created #13 to track this.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9b7c065 on kezabelle:feature/settings_checkers into * on mwarkentin:master*.

@kezabelle
Copy link
Contributor Author

Finally found time to do the work discussed in previous comments.

@mwarkentin
Copy link
Owner

Great! I'll try to take a look this afternoon.

On Friday, May 30, 2014, Keryn Knight [email protected] wrote:

Finally found time to do the work discussed in previous comments.


Reply to this email directly or view it on GitHub
#12 (comment)
.

@mwarkentin
Copy link
Owner

@kezabelle Sorry about the delay. 😦

I've checked out your branch locally, and done some manual testing. I ran into an issue when attempting to customize the checks that were being run. Not sure what's causing it, but when I try to only run the caches_status check, it responds with the databases_status check instead.

settings.py

WATCHMAN_CHECKS = (
    'watchman.checks.caches_status',
)

GET https://my.wave-local.com/watchman/

{"databases": [{"default": {"ok": true}}]}

Other than that, restricting the checks via query params seemed to work well.

Going to experiment with writing a custom check in a separate module and play around with that for a while.

If we can fix the issue with the default checks I mentioned above, I think it's just a matter of updating the documentation, and then I'm happy to put out a 0.2.0 release.

Thanks again!

@mwarkentin
Copy link
Owner

I haven't been able to figure out what's going on with that error - almost seems like something is being cached for some reason.

return attr


def get_checkers(paths_to_checkers):
Copy link
Owner

Choose a reason for hiding this comment

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

Rename to get_checks / paths_to_checks.

@mwarkentin
Copy link
Owner

@kezabelle I've created a separate PR (#15) on a branch of my own to make the changes I suggested - hope you don't mind! I'm not sure if this is the normal way for me to tweak your changes.

@kezabelle
Copy link
Contributor Author

Hey, that's absolutely fine. The trouble with GitHub is it's not possible (AFAIK) to return a notification to unread-status for dealing with later, and I evidently forgot about it as I wasn't immediately able to look at it. I'm sorry about that.

Did you manage to establish what was wrong that was causing a cache-like response?

@mwarkentin
Copy link
Owner

@kezabelle Haven't been able to track that down yet.

@mwarkentin
Copy link
Owner

It's the strangest thing - only seems to be caching if I have both lines in the settings, and comment / uncomment them out to switch. If I have a single setting, which I manually change between them - it will update correctly.

I guess if I'm just adding and removing a single # from each line, the file size wouldn't be changing.. maybe that's causing something to not update on Django's side? The weird part is that even after cleaning pycs and restarting the process.. still doesn't update.

Any guesses? In any case, I'm not sure this is actually an issue with the watchman code..

@mwarkentin
Copy link
Owner

Basically, switching between these won't update:

WATCHMAN_CHECKS = (
    # 'watchman.checks.caches_status',
    'watchman.checks.databases_status',
)
WATCHMAN_CHECKS = (
    'watchman.checks.caches_status',
    # 'watchman.checks.databases_status',
)

But this will:

WATCHMAN_CHECKS = (
    'watchman.checks.caches_status',
)
WATCHMAN_CHECKS = (
    'watchman.checks.databases_status',
)

@mwarkentin
Copy link
Owner

Also confirmed that commenting one out, and the other in, and adding a space to the commented out one causes a correct reload.

So weird.

@mwarkentin
Copy link
Owner

I've made a small video showing the issue: https://s3.amazonaws.com/snaps.michaelwarkentin.com/settings-weirdness.mp4

@mwarkentin
Copy link
Owner

This is taken care of by #15.

@mwarkentin mwarkentin closed this Sep 4, 2014
@kezabelle
Copy link
Contributor Author

That's an interesting quirk. I couldn't begin to tell you why that's happening, but I'll try and remember to look into it and/or look out for it.

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

Successfully merging this pull request may close these issues.

3 participants