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

feat: Add edxapp celery worker services (optional) #69

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

timmc-edx
Copy link
Member

Copied worker configs from Confluence with the following changes:

  • Removed newrelic integration
  • Added mysql80 dependency for workes (not just mysql57)
  • Updated image to match the newer coordinates (matching lms and cms containers, which apparently both use lms-dev)
  • Added explanation to C_FORCE_ROOT
  • Fixed cms-worker hostname (was using lms in name)

Supporting changes:

  • Add docs and comment explaining how to use the workers
  • Add cms-worker and lms-worker to the list of services available for CLI autocomplete

Some mostly unrelated cleanup as well:

  • Update lms and cms Django settings argument from devstack_docker to just devstack; the former only imported the latter and that has been the case for years (after the switch from Vagrant).
  • Alphabetize service lists (cms was renamed from studio and should have been moved)
  • Fix some link syntax in the service list

I've completed each of the following or determined they are not applicable:

  • Made a plan to communicate any major developer interface changes (or N/A)
    • Will announce in devstack channel

Copied worker configs from Confluence with the following changes:

- Removed newrelic integration
- Added mysql80 dependency for workes (not just mysql57)
- Updated image to match the newer coordinates (matching lms and cms
  containers, which apparently both use `lms-dev`)
- Added explanation to `C_FORCE_ROOT`
- Fixed cms-worker hostname (was using `lms` in name)

Supporting changes:

- Add docs and comment explaining how to use the workers
- Add cms-worker and lms-worker to the list of services available for
  CLI autocomplete

Some mostly unrelated cleanup as well:

- Update lms and cms Django settings argument from `devstack_docker`
  to just `devstack`; the former only imported the latter and that has
  been the case for years (after the switch from Vagrant).
- Alphabetize service lists (cms was renamed from studio and should have
  been moved)
- Fix some link syntax in the service list
Copy link
Member

@dianakhuang dianakhuang left a comment

Choose a reason for hiding this comment

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

Just one minor question that's not blocking.

hostname: lms-worker.devstack.edx
depends_on:
- mysql80
- mysql57
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need both of these? I'm assuming you included them because it's already how lms and cms already do them.

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea! Just copying what's done in lms and cms, like you say. I assume someone will come through and do cleanup at some point and I figured it would be best to leave it as close to the other instances as possible.

@timmc-edx timmc-edx merged commit 05b4af4 into master Nov 8, 2024
10 of 11 checks passed
@timmc-edx timmc-edx deleted the timmc/edxapp-workers branch November 8, 2024 21:49
@nsprenkle
Copy link
Member

@timmc-edx , this PR introduces an issue running make dev.up. Please advise.

"services.cms-worker": container name "edx.devstack.lms-worker" is already in use by "services.lms-worker": invalid compose project
make[1]: *** [dev.pull.frontend-app-learning+lms+cms] Error 15
make: *** [dev.pull] Error 2

@timmc-edx
Copy link
Member Author

Oops, I see I missed a spot. PR to fix: #70

Interestingly, I don't get that symptom... maybe some difference between Mac and Linux docker. But this should fix it either way.

@timmc-edx
Copy link
Member Author

@nsprenkle Merged -- let me know if that fixes it for you.

@nsprenkle
Copy link
Member

@timmc-edx, yes that seems to work for me :)

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