Initialize codejail_service application #2
Conversation
timmc-edx
force-pushed
the
timmc/init
branch
2 times, most recently
from
eba090b to
e0e2474
Compare
Makefile
Outdated
| detect_changed_source_translations: ## check if translation files are up-to-date | ||
| cd codejail_service && i18n_tool changed | ||
|
|
||
| validate_translations: fake_translations detect_changed_source_translations ## install fake translations and check if translation files are up-to-date | ||
|
|
||
| docker_build: |
There was a problem hiding this comment.
As noted elsewhere (in Slack), I'm unsure what kind of basic in-repo Docker support is still wanted at this point. I think it is fine to delete this while we don't know, and just pointing out that I don't know.
There was a problem hiding this comment.
Here's the devstack DEPR: openedx/public-engineering#247
It describes removing all of the devstack.py from repos, and contemplates removing all of the docker elements as well, since it's unused in Tutor. I think we want to get out ahead of that.
There was a problem hiding this comment.
Start reading the conversation starting here: openedx/public-engineering#247 (comment)
There was a problem hiding this comment.
@timmc-edx: I don't think this conversation is resolved yet.
- Please see my last comment.
- I don't think this requires any changes to your PR, but I do think it requires a follow-up communication/discussion somewhere about this open question. Let me know if you will initiate that, or if you need me to. Thank you.
There was a problem hiding this comment.
Oh, I misunderstood the meaning of your previous comment. I see now that there might be a role for a base development file that Tutor and Devstack can both build on. But it looks like Diana is already involved in the broader discussion?
There was a problem hiding this comment.
Sure. I don't know how this (or any other) new service should be treated at this time, and who might be keeping track (if needed) of the services that will need to be updated once they come to resolution.
codejail_service/urls.py
Outdated
| re_path(r'^api/', include(api_urls)), | ||
| re_path(r'^api-docs/', get_swagger_view(title='codejail-service API')), |
There was a problem hiding this comment.
Could this be useful in a development environment?
There was a problem hiding this comment.
Hmm... I suppose. I could bring some of these things back in for consistency with other IDAs.
There was a problem hiding this comment.
Looks like this maybe requires a DB.
requirements/base.in
Outdated
| django-extensions | ||
| django-rest-swagger | ||
| django-waffle | ||
| djangorestframework |
There was a problem hiding this comment.
I don't know if DRF will still make sense for consistency or not.
| @@ -45,8 +41,13 @@ def root(*path_fragments): | |||
| # Python dotted path to the WSGI application used by Django's runserver. | |||
| WSGI_APPLICATION = 'codejail_service.wsgi.application' | |||
|
|
|||
| # No database for codejail | |||
| DATABASES = {} | |||
| # Django *really* wants a database. So we give it a blank in-memory one. | |||
There was a problem hiding this comment.
Do we want to add any sort of warning in the README that the service should be treated as-if it has no database?
There was a problem hiding this comment.
It's mentioned in the README -- do you think it should be more prominent?
There was a problem hiding this comment.
Yes. And I don't just mean that the service does not have a database, but a warning about the required db for Django. For example, something like:
Warning: This service is configured with an in-memory database simply to make Django happy. The service itself should be considered to not to have a database, and should not rely on any database-dependent features, like waffle-based toggles, as one example.
There was a problem hiding this comment.
Ah, I see. Made some changes, take a look.
Makefile
Outdated
| detect_changed_source_translations: ## check if translation files are up-to-date | ||
| cd codejail_service && i18n_tool changed | ||
|
|
||
| validate_translations: fake_translations detect_changed_source_translations ## install fake translations and check if translation files are up-to-date | ||
|
|
||
| docker_build: |
There was a problem hiding this comment.
@timmc-edx: I don't think this conversation is resolved yet.
- Please see my last comment.
- I don't think this requires any changes to your PR, but I do think it requires a follow-up communication/discussion somewhere about this open question. Let me know if you will initiate that, or if you need me to. Thank you.
- Use cookiecutter version of .gitignore - Drop existing LICENSE file in favor of LICENSE.txt provided by cookiecutter. Basically same contents, but different path. LICENSE.txt is what we use elsewhere. Only contents difference is that cookiecutter version doesn't include the "how to use this license" appendix.
|
Rewrote commits to squash fixups but preserve the cookiecutter/changes distinction, and edit commits. Cleaned up license file move/move-back in commit history. Only change to diff is that the license file no longer includes the "APPENDIX: How to apply the Apache License to your work." section that we usually omit. |
Major changes: - Trim dependencies, settings, and application code to remove anything relating to having a database (auth, models, pii-annotations, reserved-keywords) or being exposed to the outside world (cors, csrf). Add an in-memory DB for to satisfy Django's need for a "database". - Remove unneeded requirements files. We're going to deploy the base layer instead of the production layer, and requirements.txt is just not needed at all as far as I can tell. - Install gunicorn in the base layer. It's not clear if we'll want to use it from devstack (or just use runserver) but it should be available when we want to use it. - Don't hardcode gunicorn's bind address; we'll pass it as `--bind` from the Dockerfile. - Remove Dockerfile and devstack.py as these will be provided elsewhere. Fixes: - Use Python 3.12 for tox invocation (to match tox.ini and others) Docs: - Fill out README. Full documentation has been ticketed in #3 and linked from the README. - Removed PyPI badge as this is an IDA and won't be published as a package. - Update catalog-info.yaml from defaults - Tweaks to PR template Build: - Suppress code coverage checks for gunicorn config file Finally: - Run `make upgrade`
Two commits, best reviewed individually:
This is part of edx/edx-arch-experiments#894