Hi @jflory7! This PR solved the issue #181. I have added the following in this PR:

1. Auto reload of code changes without having to build containers again.
2. Documentation update with the same.
3. Resolved a bug in templates/search/search.html, HTML was not escaped.

I would love to know your thoughts on this one! Hopefully we'll speed up the development process with this :smile:

Also, please let me know if merging the two commits makes more sense here.

There are two issues with mounting this volume:

1. For SELinux-enabled systems, the container will not be able to access files without an extra :z appended (see Docker docs – this blog post is also interesting but it was written four years ago so take it with a grain of salt)
2. Copying the current directory overrides the container build process

Instead, I think it might be better to mount the ./assets/ directory instead (I realize I advised against this a few months ago, but I didn't have a full understanding of how things were set up here at the time).

I realize now this instruction is incorrect. In the container build instructions (i.e. Dockerfile), it generates client secrets whether asked or not. When we mount the volume in docker-compose.yml, it replaces all files copied into the container in the Dockerfile (even if some files are missing). In our case for development, I think it's better to omit this line for documentation along with the volume mount feedback above.

Hi @shraddhaag! Thanks for opening this. :raised_hands:

I left a couple of in-line comments above. I realize what I advised before was probably not the best advice. In PR #193, @revanth added two volume paths for the static content on our site, like site assets (JS, images, CSS) and the HTML template paths. Since these are frequently edited and changed, I think these paths make the most sense to mount and re-generate on the fly. Mounting the entire app folder may cause issues with some files like the client secrets.

I'm leaning towards requiring a rebuild of the containers if underlying changes are made to the back-end with Django. I think this will help us keep our development and production containers more in sync in the long run, but this is my hypothesis, not a hard truth.

What do you think? Does this feedback make sense?

For SELinux-enabled systems, the container will not be able to access files without an extra :z appended

I didn't know about this. Thanks for pointing it out!

Copying the current directory overrides the container build process

That makes sense. I missed this one completely.

I realize now this instruction is incorrect. In the container build instructions (i.e. Dockerfile), it generates client secrets whether asked or not.

I'm not sure if I'm understanding this right. The Dockerfile first has the instruction for copying the entire PWD in the container (including client_secrets.json if it exists). Then it runs the script generate_client_secrets.sh. In this file it is first checked if a client_secrets.json is already present or not, and if not present it is only then generated.

I verified this by checking the time stamp on the client_secrets.json inside the container [1557113979 = Mon May 6 03:39:39 2019] and the following is logged while building the containers using Dockerfile:

Step 11/13 : RUN ./generate_client_secrets.sh
 ---> Running in 722df65b4991
client_secrets.json already exists. To regenerate, delete the file.
Removing intermediate container 722df65b4991

I tested the above after removing all volumes.

This leads me to believe that client_secrets.json is only generated when not already present. Please let me know if I interpreted this wrong.

In PR #193, @revanth added two volume paths for the static content on our site, like site assets (JS, images, CSS) and the HTML template paths. Since these are frequently edited and changed, I think these paths make the most sense to mount and re-generate on the fly. Mounting the entire app folder may cause issues with some files like the client secrets.

I've added the above volumes in docker-compose.yml.

I'm leaning towards requiring a rebuild of the containers if underlying changes are made to the back-end with Django. I think this will help us keep our development and production containers more in sync in the long run, but this is my hypothesis, not a hard truth.

I would also like to introduce a volume for the main app happinesspackets which will reflects the changes made to the Django back-end and will simultaneously not override the container build process. I tested the same on my end by introducing the following volume:

- ./happinesspackets:/app/happinesspackets:z

Please let me know if I should include the above or if front-end autoreload is our ideal scenario.

Hi @shraddhaag!

Also, please let me know if merging the two commits makes more sense here.

I forgot to say that the two commits was the better approach, since they are two unrelated changes. Rebasing and squashing commits makes sense when combining related changes together. If there are unrelated changes, even if one is small, I prefer to keep them as separate commits.

Since the commits are already squashed in this PR, don't worry about splitting them out again. Something to keep in mind for future PRs. :)

This leads me to believe that client_secrets.json is only generated when not already present. Please let me know if I interpreted this wrong.

Oops! You are totally right. I forgot the script checked if client_secrets.json already exists.

I would also like to introduce a volume for the main app happinesspackets which will reflects the changes made to the Django back-end and will simultaneously not override the container build process. I tested the same on my end by introducing the following volume:

- ./happinesspackets:/app/happinesspackets:z

Please let me know if I should include the above or if front-end autoreload is our ideal scenario.

This makes sense to me. When you tested it, you made a change to the back-end and saw it reflected in the app? If yes, then this sounds like the right idea to me. :thumbsup:

Hi @jflory7!

I forgot to say that the two commits was the better approach, since they are two unrelated changes. Rebasing and squashing commits makes sense when combining related changes together. If there are unrelated changes, even if one is small, I prefer to keep them as separate commits.
Since the commits are already squashed in this PR, don't worry about splitting them out again. Something to keep in mind for future PRs. :)

This makes sense. I'll make sure to keep this in mind for future :)

This makes sense to me. When you tested it, you made a change to the back-end and saw it reflected in the app? If yes, then this sounds like the right idea to me. 👍

Yes, the changes to the back-end (eg- changing the URL path) were reflected in the app. I've added the same and modified the docs again to accommodate the same :)

What started as a small issue definitely ended up making me learn a lot of new things about Docker! Hopefully we can get this one merged now :smile:

Nice work on figuring this one out @shraddhaag! :raised_hands: Thanks for being diligent and catching my oversights.

This looks ready to go to me. Merging! :ocean: