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

Make watchman settings importable #130

Closed
jonespm opened this issue Oct 17, 2018 · 8 comments
Closed

Make watchman settings importable #130

jonespm opened this issue Oct 17, 2018 · 8 comments

Comments

@jonespm
Copy link
Contributor

jonespm commented Oct 17, 2018

The django-debug-toolbar had a fix a few years ago to make it "import safe" django-commons/django-debug-toolbar#801

This basically changed up the settings so that anything calling getenv wasn't called directly at the time. This is useful to be able to import the default attributes and configuration from a local settings

For instance this doesn't seem to be possible in (local) settings.py

collectstatic/django will either exception out about SECRET_KEY or even if this is imported after SECRET_KEY is defined then the locally defined WATCHMAN_CHECKS aren't set.

from watchman import settings as wm_settings

. . .

WATCHMAN_CHECKS = wm_settings.DEFAULT_CHECKS + ('dashboard.common.watchman.custom_checks.check_crontab', )

I think either the "constant's" could be moved to a separate constants file or the fix similar to the fix for 801 could be used. The workaround is just to redefine the the full set of DEFAULT_CHECKS locally but those "might" change in a future version and then would automatically update!

Thanks!

@mwarkentin
Copy link
Owner

Is this similar to what was discussed in #13? I'd be open to a PR to improve this - extracting into a standalone constants.py seems like it would be the simplest approach. Any downsides?

@jonespm
Copy link
Contributor Author

jonespm commented Oct 18, 2018

I can't see any downsides. Watchman settings would import this, so anyone that current is (somehow?) importing the settings directly wouldn't be affected. Would be less changes than the refactor on toolbar.

@jonespm
Copy link
Contributor Author

jonespm commented Oct 18, 2018

I created a simple PR for just moving out these 2 constant tuples to a separate file, tested it locally and updated the README.

mwarkentin added a commit that referenced this issue Oct 19, 2018
Make watchman constants importable Fixes #130
@mirceaciu
Copy link

small impediment (maybe?). I just did a pip install django-watchman and this merge does not seem to be built (I don't have the separate constants file)

image

@jonespm
Copy link
Contributor Author

jonespm commented Mar 19, 2019

Yeah, it looks like there hasn't been a new version released since 0.15.0 on Feb 2018...

It looked like I'd locally been installing and using this still as something like
pip install git+https://github.com/mwarkentin/django-watchman@master#egg=django-watchman

And didn't notice there wasn't a new version released yet.

GitHub
GitHub is where people build software. More than 31 million people use GitHub to discover, fork, and contribute to over 100 million projects.

@mirceaciu
Copy link

mirceaciu commented Mar 19, 2019

my comment seems to relate to #136

@mwarkentin
Copy link
Owner

Oops, sorry about that. I'll try to get a new release out shortly!

@mwarkentin
Copy link
Owner

I've pushed django-watchman 0.16.0 to pypi.. hope that helps!

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

No branches or pull requests

3 participants