Skip to content

Feature/celery 4976-In Review #5110

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.

Sign up for GitHub

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

Jump to bottom
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

@androolloyd androolloyd commented Aug 31, 2019

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

@androolloyd
Celery Task Scheulder and Worker Setup
879960d 
	- 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
Merge branch 'master' of github.com:gitcoinco/web into feature/celery…
503e7a0 
…-4976
@androolloyd
Copy link
Contributor Author

androolloyd commented Aug 31, 2019

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

danlipert commented Oct 15, 2019

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

@danlipert
Copy link
Contributor

danlipert commented Oct 23, 2019

@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 •
edited
Loading

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

danlipert commented Oct 28, 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.

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 •
edited
Loading

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
androolloyd
androolloyd commented Nov 20, 2019
View reviewed changes
@androolloyd
Copy link
Contributor Author

androolloyd commented Nov 20, 2019

This is ready for deployment

@androolloyd
Copy link
Contributor Author

androolloyd commented Nov 20, 2019

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

octavioamu commented Nov 20, 2019

@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

Reviewers

3 more reviewers

@danlipert danlipert danlipert approved these changes

@octavioamu octavioamu octavioamu approved these changes

@thelostone-mc thelostone-mc thelostone-mc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@androolloyd @danlipert @octavioamu @thelostone-mc