JM's suggested tweaks based on rfds upgrade #26
Conversation
Push the built but pre-test Docker image in parallel with the tests run so that the image will be available for fetching from Docker Hub if necessary, e.g. if you re-run the Codefresh run, or want to fetch and debug a failing image locally. This is mainly motivated by the fact that Codefresh no longer provides its own repository for images, which it used to, and thus no longer caches every built image including those that failed the `run_tests` step.
These are the constraints for ixc-django-docker's dependencies that worked for the RFDS site that uses Django 1.8, when re-creating that project's pinned requirements.txt file from scratch.
DOTENV is useful in many places, such as setting unique names for things like databases or ElasticSearch indexes etc. Make it easily available as a setting that is guaranteed to be present, and given a clear default value if not, instead of requiring projects to hunt for and handle the 'DOTENV' environment variable.
| exit 1 | ||
| fi | ||
|
|
||
| rm -rf "$DIR/var" |
There was a problem hiding this comment.
I think this should be $PROJECT_DIR?
Also, this can only run from within an active project shell where the virtualenv has been configured. But it will completely delete the virtualenv. You'll need to exit the shell and re-run ./go.sh again to recreate it, steps that I don't think we can automate. For a full reset, there are also other directories that could be removed. bower_components, node_modules, static_root (which includes compiled/compressed assets as well as collected files). Also the database.
That's why I never created a reset script. Too many variables. I figured people can just reset just what they need to reset when they need it.
There was a problem hiding this comment.
You can also already do SETUP_POSTGRES_FORCE=1 setup-postgres.sh (drop/recreate db from initial_data.sql dump or creds in SRC_* env vars) and SETUP_FORCE=1 setup.sh (remove *.md5 files first). node_modules dir is also already removed when reinstalling. Nothing to delete var, though.
There was a problem hiding this comment.
@mrmachine Maybe the nuke commands could be printed somewhere obvious when the ./go.sh script runs? So they are available for copy-and-paste, but don't try to automate more than we reasonable can?
There was a problem hiding this comment.
An alternate approach: ./go.sh --clean: https://github.com/ixc/rfds/commit/df08700e5d4a5a079522f99d1d37fd04002a233c
| @@ -20,6 +20,9 @@ | |||
|
|
|||
| from django.utils.six import text_type | |||
|
|
|||
| # Store DOTENV from environment as a setting | |||
| DOTENV = os.environ.get('DOTENV', 'dotenv-not-set') | |||
There was a problem hiding this comment.
Why do we need this?
There was a problem hiding this comment.
See my commit message around that. It's very useful, and I'm stick of having to look up os.environ['DOTENV'] to set things like ElasticSearch index name
| @@ -20,7 +20,7 @@ project_settings_local.py | |||
| src/ | |||
| static_root/ | |||
| var/ | |||
| venv | |||
| venv/ | |||
There was a problem hiding this comment.
This was intentional. The virtualenv is stored in the var directory anyway, but I create a symlink to it from venv so VSCode can automatically find it. We could remove this and I cold add venv to my global ignores, instead?
There was a problem hiding this comment.
Yeah, if it's only there for you @mrmachine better to remove it
| # Safely send real emails; Bandit prevents sending to arbitrary addresses | ||
| export BANDIT_EMAIL='[email protected]' | ||
| export BANDIT_WHITELIST='interaction.net.au,[email protected]' |
There was a problem hiding this comment.
Comment these out by default? Won't work without SMTP credentials either, which are usually not set in .env.develop.secret.
There was a problem hiding this comment.
@mrmachine I really want to ensure we have a safe email setup by default. You are right that these settings are pointless without corresponding SMTP settings, but as soon as anyone adds SMPT settings the email hijacking will apply and prevent unintended emails.
If they're commented out, there's even less chance anyone will ever apply these butt-saving settings.
| composition: | ||
| version: '2' | ||
| # Run tests and push built image in parallel | ||
| in_parallel: |
There was a problem hiding this comment.
Name this step push_and_test_image or something? Step names are unique. Some project might want another parallel step one day (in fact we have another parallel step already further down).
There was a problem hiding this comment.
🤷 If you want, I don't care about the name
| PGUSER: postgres | ||
| REDIS_ADDRESS: redis:6379 | ||
| image: '${{build_image}}' | ||
| # Push UNTESTED image to improve caching (not to be used directly) |
There was a problem hiding this comment.
Add "and speed up the pipeline". That's the second and equally (or possibly even more) impactful optimisation (affects every build, not only future builds that can use build cache).
There was a problem hiding this comment.
The pipeline speed-up is a side-effect of running that push in parallel, so I don't think that point belongs in the comment above just the push step
| ############################################################################## | ||
| # Django<1.11 compatibility recommendations | ||
|
|
||
| # -e git+https://github.com/ixc/ixc-whitenoise.git@9812e7b3caa62a438c9332653c2b297856d6f104#egg=ixc-whitenoise # Compatibilty with older Django | ||
| # celery<4.3 # Version 4.3 requires Django>=1.11 | ||
| # django-appconf<1.0.4 # Version 1.0.4 should work with Django 1.8 but doesn't | ||
| # django-celery-beat<1.6 # Version 1.6 requires Django>=1.11.17 | ||
| # django-celery-email<3 # Version 3.0.0 requires Django>1 | ||
| # django-celery-results<1.1 # Version 1.1 requires celery>=4.3 | ||
| # django-compressor<2.2 # Version 2.3 requires Django>1.11, but 2.2 doesn't work | ||
| # django-debug-toolbar<1.10 # Version 1.10 requires Django>=1.11 | ||
| # django-extensions<2.1.1 # Version 2.1.1 requires Django>=1.11 | ||
| # django-ipware<3 # Version 3 requires Python>=3.5 | ||
| # django-post-office<3.3 # Version 3.3.0 requires Django>=1.11 | ||
| # django-redis<4.9 # Version 4.9.0 requires Django>=1.11 | ||
| # django-storages<1.6.6 # Version 1.6.6 requires Django>=1.11 | ||
| # django-timezone-field<3.1 # Version 3.1 requires Django>=1.11 | ||
| # jsonfield<2.1 # Version 2.1 requires Django>=1.11 | ||
|
|
||
| ############################################################################## | ||
|
|
||
|
|
||
| ############################################################################## | ||
| # Use Thumbor for thumbnails (latest versions of IC forks of Thumbor libs) | ||
|
|
||
| # -e git+https://github.com/ixc/django-thumbor.git@481cf39536b384369b2bc31da2af2bee34a4c617#egg=django-thumbor | ||
| # -e git+https://github.com/ixc/libthumbor.git@d5f44bbd6cd7c19f61f47d941213d1de52209521#egg=libthumbor | ||
|
|
||
| ############################################################################## |
There was a problem hiding this comment.
Can we put these in an SOT post and link to it from a comment, with links to different Django versions?
Might be more generally discoverable there and useful to anyone upgrading deps, not necessarily as a whole stack/ixc-django-docker upgrade. And I don't think this is relevant to anyone making a new project with the project template (probably using latest versions).
There was a problem hiding this comment.
I would rather not move them. Here they are present exactly where they can be easily enabled for anyone upgrading old sites (which in theory we are doing?) and version controlled. I'm not going to remember to return to SOT every time I find a tweak to settings like this, whereas I might remember to upstream them to the ixc-django-docker.
I'm in favour of linking from a SOT post to this listing.
* master: Do not match pyenv error string accidentally, which says the required version is NOT installed. Fix flipped bool test. Actually exit when dependencies are not met. Don't specify base image version in comments. Remove extra blank line. Export `CPATH` so zlib headers can be found from Xcode while building wheels in Catalina.
The `admin_tools` app comes from django-admin-tools which isn't referenced anywhere else in ixc-django-docker, so this was probably added to the default URLs by mistake.
For RFDS the site admin is under /rfds-admin/ not /admin/. The new ADMIN_URL setting makes the admin URL prefix easily configurable in downstream projects without needing to mess with URL patterns.
* master: Add missing `revision` to main clone step.
jmurty
changed the title
This step is redundant in setup.sh because it gets run as part of the recommended Dockerfile when building an image. Before this change static files etc would get generated by the Codefresh build, then again slowly and pointlessly when you run the setup service. So either this command should be removed altogether from this script, or maybe be adjusted to run only in a local development context?
No description provided.