pretenders / ployst

The ployst core repo
1 stars 0 forks source link

Kick off async process on github posts. Add command for easier testing. #23

Closed alexcouper closed 10 years ago

alexcouper commented 10 years ago

This PR contains some controversial elements.

The main two being:

I began by writing a celery app just within the github app, but decided that it made more sense for the github app to define the tasks it needs, but not be responsible for running celery itself.

Additionally, if settings is just seen as the final configuration layer when setting up ployst, and it can contain settings like GITHUB_HOOK_TOKEN, then it makes sense for github to be an app listed in INSTALLED_APPS. (And this had the benefit of making it exceptionally easy to use a centralized celery app).

I think that we should, however, continue to develop against the RESTful API - despite the fact that performance wise it makes more sense for the client to be using django models directly. The reason being it will force us to make the API robust and this will certainly be useful when extending the angular client.

alexcouper commented 10 years ago

On reflection, this isn't so controversial after all.

If we ever want to run the providers separately they will need to be run with a settings file that names them in installed apps, and have their own celery runner. But these are reasonable constraints to have.

alexcouper commented 10 years ago

@txels when you get a moment ;)

txels commented 10 years ago

Acknowledged. Will review shortly. I don't think there is much controversy really, seems like a reasonable approach.

txels commented 10 years ago

It all looks good to me. I added some comment/question, happy to merge on pingback. BTW there is a GITHUB_HOOK_TOKEN defined on settings.dev - this is probably your local token? It is not the one I have now, so I forgot when that was added there. This should probably go in personal settings.

e.g. I have now ployst/settings/dev_cba.py:

from .dev import *  # noqa
GITHUB_HOOK_TOKEN = "27665cd2-c386-4fa6-a741-656fcf23ca65"
alexcouper commented 10 years ago

Yeah - personal settings is where that should live. I must have committed by mistake.

alexcouper commented 10 years ago

Remind me how you're getting your personal settings to be used?

Are you doing: DJANGO_SETTINGS_MODULE=ployst.settings.dev_cba python manage.py runserver

txels commented 10 years ago

Yes either that or export DJANGO_SETTINGS_MODULE=ployst.settings.dev_cba at the start (e.g. you can use a virtualenvwrapper post-activate hook for that (some colleagues do) I then to have a script mysettings.sh that I run . ./mysettings.sh when I need to.

txels commented 10 years ago

Fine to merge. You can decide when to remove that print statement.