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

Feature/celery 4976-In Review #5110

Merged
merged 6 commits into from
Nov 20, 2019
Merged

Feature/celery 4976-In Review #5110

merged 6 commits into from
Nov 20, 2019

Conversation

androolloyd
Copy link
Contributor

Description

Celery Task Scheulder and Worker Setup

  • dashboard/tasks.py
    • bounty_email task
      • locks on the invite_url
        requirements/base.txt
    • django_celery_beat depdency added
    • celery dependency

RedisService added
Settings.py

  • celery specific env variables added

docker-compose.yml

  • worker and scheduler added
Refers/Fixes

#4976

Testing

Testing Still needed

	- dashboard/tasks.py
	  - bounty_email task
	    - locks on the invite_url
	- django_celery_beat depdency added
	- celery dependency

	RedisService added
	Settings.py
	- celery specific env variables added

	docker-compose.yml
	- worker and scheduler added
@androolloyd
Copy link
Contributor Author

Some questions remain around sending emails, I noticed there are some administrator features around sending bounty emails, all aspects of sending bounty emails will need to be tested end 2 end.

@danlipert
Copy link
Contributor

@androolloyd whats left to do on this one so we can get it merged, hopefully by Wednesday?

@danlipert
Copy link
Contributor

@androolloyd any updates on this? Also, I'm wondering why we need the scheduler here since we aren't actually doing scheduling? Is it just so we can space out the emails over time?

@androolloyd
Copy link
Contributor Author

androolloyd commented Oct 23, 2019

The scheduler could be turned off for now i suppose, it executes what gets defined in the database for periodic tasks.

IIRC its not required for the email job to function.

@danlipert
Copy link
Contributor

The scheduler could be turned off for now i suppose, it executes what gets defined in the database for periodic tasks.

IIRC its not required for the email job to function.

cool, in that case lets remove it for now so we don't prematurely grow the codebase

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #5110 into master will increase coverage by 1.74%.
The diff coverage is 60.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5110      +/-   ##
==========================================
+ Coverage   29.12%   30.86%   +1.74%     
==========================================
  Files         242      247       +5     
  Lines       20912    23441    +2529     
  Branches     3023     3704     +681     
==========================================
+ Hits         6091     7236    +1145     
- Misses      14576    15880    +1304     
- Partials      245      325      +80
Impacted Files Coverage Δ
app/marketing/mails.py 13.74% <0%> (-0.04%) ⬇️
app/dashboard/tasks.py 50% <50%> (ø)
app/app/redis_service.py 90% <90%> (ø)
app/app/settings.py 80.56% <92.85%> (+0.91%) ⬆️
app/taskapp/celery.py 93.33% <93.33%> (ø)
app/app/urls.py 85.93% <0%> (-4.07%) ⬇️
app/grants/views.py 15.53% <0%> (-0.8%) ⬇️
...rketing/management/commands/no_applicants_email.py 0% <0%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8eed88...4426cf3. Read the comment docs.

@androolloyd androolloyd changed the title Feature/celery 4976-Needs-More-Testing Feature/celery 4976-In Review Nov 15, 2019
@androolloyd androolloyd marked this pull request as ready for review November 15, 2019 14:52
app/taskapp/celery.py Outdated Show resolved Hide resolved
app/marketing/mails.py Outdated Show resolved Hide resolved
@androolloyd
Copy link
Contributor Author

This is ready for deployment

@androolloyd
Copy link
Contributor Author

The workers are running on the celery server, once the app has been updated it should start queueing jobs to redis. The worker box should get switched back to master and get updated to the latest version as apart of the deploy.

@octavioamu
Copy link
Contributor

@androolloyd I noticed the file app/assets/v2/js/lib/secrets.min.js was changed to unminified file, probably another version. is that ok? Also shouldn't be good to put a minified version?

@octavioamu octavioamu merged commit b308659 into master Nov 20, 2019
@androolloyd androolloyd deleted the feature/celery-4976 branch November 20, 2019 14:32
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.

4 participants