Conversation
naglepuff
force-pushed
the
migrate-django-config
branch
4 times, most recently
from
0e4ea56 to
d88a5b6
Compare
naglepuff
force-pushed
the
migrate-django-config
branch
3 times, most recently
from
ea5ecf7 to
f33b42c
Compare
naglepuff
force-pushed
the
migrate-django-config
branch
from
f669078 to
e3f8210
Compare
We aren't using it for this project at the moment.
naglepuff
force-pushed
the
migrate-django-config
branch
from
0f46812 to
3aed18a
Compare
Co-authored-by: Brian Helba <[email protected]>
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
Could you remove the ./.tox file from this PR?
Some other minor questions/changes
DEPLOYMENT.md
Outdated
|
|
||
| Then run `docker compose -f docker-compose.prod.yml run \ | ||
| --rm django ./manage.py importGRTSCells /app/csv/grts.csv` | ||
| --rm django ./manage.py importGRTSCells /opt/django-project/app/csv/grts.csv` |
There was a problem hiding this comment.
The mounting location for /app/csv/grts.csv needs to by synced between the overrride docker compose in the root of the proejct and the ./prod/docker-compose.prod.yml.
docker-compose.override.yml - has it at this location where it will be under /opt/django-project/app/csv/grts.csv
./prod/docker-compose.prod.yml - has it mounted at /app/csv/grts.csv
We needs these to probably be consistent or have two different instructions.
There was a problem hiding this comment.
I think we should just be able to change the /app/ part to /dev, and remove the mount entirely. The prod dockerfile copies everything in its context into the /opt/django-project directory, so as long as it is built with that file in context, it should work ok.
For dev/the override, we mount the root directory to /opt/django-project, so I think it would just be the same command for both. I also see this as the only reference to running the command in the codebase. Maybe we should add something to the root's README?
There was a problem hiding this comment.
Yeah if you don't mind adding someting.
| def get_or_create_processing_task(request_id): | ||
| """ | ||
| Fetches or creates a ProcessingTask with the given metadata and status filters. | ||
| Fetch or creat a ProcessingTask with the given metadata and status filters. |
There was a problem hiding this comment.
creates? Althought right now I'm noticiing this being used in multiple places, may be something that I refactor out into a utils location.
| @@ -1,23 +1,2 @@ | |||
| .DS_Store | |||
There was a problem hiding this comment.
You can leave it as is, but I'm curious about the adjustment to move client-specific ignores to the root while still maintaining this one for local .env files.
There was a problem hiding this comment.
We could probably get rid of the client/.gitignore altogether. Any objections to that?
| image: traefik | ||
| container_name: traefik | ||
| env_file: ./dev/.env.prod.docker-compose | ||
| env_file: ./.env.kitware-production.template |
There was a problem hiding this comment.
further down: https://github.com/Kitware/batai/pull/181/files#diff-b23aa267b8a3bfc3bcca0f7f5b84b380f6b1b1ac8ff559abca8e3111ac5906e1R33
You changed the location of this file that is mounted so it needs to be updated
There was a problem hiding this comment.
I'm confused, isn't the template env file in the same directory as this docker compose file?
Try to bring it more in line with the tox config.
This reverts commit 5166815.
naglepuff
force-pushed
the
migrate-django-config
branch
from
a790397 to
812050c
Compare
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
Just one comment about the node_modules and permissions but I don't think it needs to be resolved in this PR.
After this is merged I may have some other minor PR's as try to deploy in the different modes on the batdetectai.kitware.com server and attempt the subpath elements as well.
| # If node_modules is not writable by others, make it so the host can modify and delete them | ||
| [ \"\$(stat -c '%A' node_modules | cut -c9)\" != 'w' ] && chmod o+w node_modules; | ||
| # Always run the dev server |
There was a problem hiding this comment.
I thought I added this to the last review but maybe I forgot to click to add the comment. This was done so that the host system when launched had access to delete and modify the node_modules folder without requiring a sudo chown -R ./client/node_modules. If you want to delete the node_modules or npm install new dependencies the host machine won't have write access to the folder because inside the container npm install is run by root.
This is similar to how django manage.py migrate creates a migration file using the root user. We can leave this for now. I'll see if I can find a way to make this easier for both instances.
There was a problem hiding this comment.
Maybe @brianhelba can help refresh my memory here, but I believe that adding the /app/node_modules as a volume gets around this issue.
There was a problem hiding this comment.
yeah that means that it doesn't include a node_modules within the host. It allows the npm install to work in terms of editing the package and package-lock but won't actually install the dependency locally. I think it's fine for now and I'll review only if I start hitting annoying issues (which hopefulyl shouldn't happen often)
This is used by eslint, so we'll keep it for now.
Remove large image related packages
2 participants
Remove
django-composed-configurationand replace withdjango-resonant-settings. A lot of this is a copy/paste job pulling from existing settings and defaults from the resonant cookiecutter.@BryonLewis please take a look when you get a chance. I'm particularly interested to know if you think I missed anything with respect to the existing deployments. The testing/dev environments seem to be set up correctly. I was able to successfully test the major features of the application with the new method of defining django settings.