fedora-commops / fedora-happiness-packets

Clone
Source Code
GIT

#235 Auto reload code changes
Merged 4 years ago by jflory7. Opened 4 years ago by shraddhaag.
fedora-commops/ shraddhaag/fedora-happiness-packets autoreflect  into  master

file modified
+7 -1 
@@ -29,4 +29,10 @@ 
 

  fas-admin-details.json
 

  

  # YAML Configuration file for django 

- config.yml 

\ No newline at end of file
 

+ config.yml
 

+ 

+ # Django-haystack files
 

+ whoosh_index/
 

+ 

+ # VS Code files
 

+ .vscode/

file modified
+5 
@@ -2,7 +2,12 @@ 
 

  services:
 

    web:
 

      build: .
 

+     restart: always
 

      command: bash -c "python manage.py migrate --noinput && python manage.py rebuild_index --noinput && python manage.py runserver 0.0.0.0:8000"
 

+     volumes:
 

+       - ./templates:/app/templates:z
 

+       - ./assets:/app/assets:z
 

+       - ./happinesspackets:/app/happinesspackets:z
 

      ports:
 

        - "8000:8000"
 

      links:

file modified
+4 -3 
@@ -32,16 +32,17 @@ 
 

  

      ./generate_client_secrets.sh
 

  

+    Although the Dockerfile runs the script to check if a client_secrets.json file is present, please generate it before starting the Docker container, so that client secrets are not being constantly generated every time the image is rebuilt.

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.

 
+ 

  #. Create a fas-admin-details.json file and add a json object with your FAS-Username and FAS-Password. See fas-admin-details.json.example.
 

  

  #. Create a config.yml file and populate it with the user's details and usernames for `ADMINS <https://docs.djangoproject.com/en/2.1/ref/settings/#admins>`_ and superuser privileges respectively.
 

  

- Although the Dockerfile runs the script to check if a client_secrets.json file is present, please generate it before starting the Docker container, so that client secrets are not being constantly generated every time the image is rebuilt.
 

- 

  In order to run the web server, alongside the Redis queue and celery worker instance, simply run ``docker-compose up``.
 

  After this, you can access the local development server at ``http://localhost:8000/`` in your web browser.
 

  

- If you have made any changes to the code, the Docker image must be rebuilt; this can be done my running ``docker-compose up --build``.
 

+ Auto reload of code changes is configured, so you shouldn't have to rebuild containers every time you make changes to the code base. 

+ If you have made any changes to the environment of the containers (eg - adding a new dependency), the Docker image must be rebuilt; this can be done my running ``docker-compose up --build``.
 

  

  Since the code is being run in a container, we must enter the shell of the container in order to run tests.
 

  Access the shell of the Docker container by running ``docker-compose exec web sh``.

file modified
+1 -1 
@@ -26,7 +26,7 @@ 
 

  </h4>
 

  

  <blockquote>
 

-     {{ result.object.message|linebreaksbr  }}
 

+     {{ result.object.message|safe }}
 

  </blockquote>
 

  

  {% empty %}

This commit adds the functionailty to auto reload code changes made to the code base without having to build containers again to see them in effect.

To see the changes in effect, existing containers need to be removed using docker container rm <CONATINER_ID> and new containers need to be build.
Now any changes made to the files are reflected on localhost by refreshing the page!

rebased onto b96d141b9046dffb06d457759aaaba653bae96c9
4 years ago

1 new commit added

  • Resolve HTML escaping on teh search index page
4 years ago

2 new commits added

  • Resolve HTML escaping on the search index page
  • Auto reload code changes
4 years ago

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.

Metadata Update from @jflory7:
- Pull-request tagged with: PASSED, improvement, needs testing, type - infra, type - summer coding
4 years ago

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?

Metadata Update from @jflory7:
- Pull-request untagged with: needs testing
- Pull-request tagged with: needs changes
- Request assigned
4 years ago

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.

rebased onto 76dbf47dca8c0c9bb4438c7e01e77068769faccc
4 years ago

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:

rebased onto 415b401450bbe91f4d852c4b0e1b6aab709e7746
4 years ago

rebased onto dccb01673e48d39cea2b577868e5728653a82447
4 years ago

rebased onto 0a017fd
4 years ago

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:

Pull-Request has been merged by jflory7
4 years ago

Metadata Update from @jflory7:
- Pull-request untagged with: needs changes
4 years ago
Changes Summary 4
+7 -1
file changed
.gitignore
+5 -0
file changed
docker-compose.yml
+4 -3
file changed
docs/setup/development.rst
+1 -1
file changed
templates/search/search.html
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.