Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker refactor ready for AWS #1176

Merged
merged 23 commits into from
Jul 24, 2023
Merged

Docker refactor ready for AWS #1176

merged 23 commits into from
Jul 24, 2023

Conversation

ahosgood
Copy link
Member

@ahosgood ahosgood commented Jul 11, 2023

Supersedes #1031

Test by checking out branch and running:

fab build && fab start

The images should get completely rebuilt, start running automatically on http://localhost:8000/ (it may take a bit of time so check your Docker logs) and be watching for Python, template, CSS and JS changes. No other commands should be required.

EDIT 2023-07-21
The server will not automatically be started unless you have AUTO_START_SERVER=true in your .env file

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #1176 (1e3083e) into develop (2f55c23) will increase coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 1e3083e differs from pull request most recent head 99fe01b. Consider uploading reports for the commit 99fe01b to get more accurate results

@@             Coverage Diff             @@
##           develop    #1176      +/-   ##
===========================================
+ Coverage    83.36%   83.45%   +0.08%     
===========================================
  Files          115      115              
  Lines         4377     4399      +22     
===========================================
+ Hits          3649     3671      +22     
  Misses         728      728              

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ahosgood ahosgood requested a review from Puththiran July 12, 2023 09:17
Copy link
Collaborator

@jamesbiggs jamesbiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey AJ, nice work :)

I've put it on platform.sh here just to double check that nothing goes funny as a result of this: https://feature-aws-migration-qtvcsiq-rasrzs7pi6sd4.uk-1.platformsh.site/ - all looks good to me on there 😃

I've got one problem with running commands in the CLI (fab sh then e.g. python manage.py createsuperuser) it seems to be running quite slow. Normally these commands are near instant, but it's currently hanging slightly (~10 seconds) before running any of these commands. It seems like it's only the python manage.py commands that are hanging - I'm thinking it might be because we've got the server constantly running in the background?

I also tried running a few commands to make sure everything was working. I ran poetry update to update to the latest Python packages to make sure that was working - when it stopped the container and restarted the web container and that's now stuck restarting in a loop:

2023-07-12 11:49:39 Skipping virtualenv creation, as specified in config file.
2023-07-12 11:49:39 Traceback (most recent call last):
2023-07-12 11:49:39   File "/app/manage.py", line 15, in <module>
2023-07-12 11:49:39     from django.core.management import execute_from_command_line
2023-07-12 11:49:39 ModuleNotFoundError: No module named 'django'

But again I assume this is coming from the constantly running server. Is there a way to stop the site from running, but have the containers running so I can run commands without the site/server on?

docs/features/feedback-mechanism.md Show resolved Hide resolved
docs/developer-guide/frontend.md Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
fabfile.py Show resolved Hide resolved
bash/dev-watch.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Co-authored-by: James Biggs <[email protected]>
@ahosgood
Copy link
Member Author

@jamesbiggs - The line responsible for running the server on startup is this one: https://github.com/nationalarchives/ds-wagtail/pull/1176/files#diff-81ab1d3980be928d58ce321fa0b4e94c90e3ff0eaad9f8cdea2e7e63490f6f34R6

I've done that so as to be as close to the production script as possible but maybe we want to comment out this line if most people would rather not have it run automatically? Keen to know what others think...

@jamesbiggs
Copy link
Collaborator

I've done that so as to be as close to the production script as possible but maybe we want to comment out this line if most people would rather not have it run automatically? Keen to know what others think...

Personally, I like to run python manage.py runserver 0:8000 so I can see any errors that pop up - how we've got this here will hide any errors - and I'm also not too keen on python manage.py migrate being run as well.. We have this after fab pull-staging/production-data anyways so that part is there - but there may also be a case where we want to start the server without running the migrations as well 🙂

@ahosgood
Copy link
Member Author

I have commented out the two lines in bash/run-dev.sh that run migrations and start the development server. Anyone can always un-comment them if they find it helpful.

Hope that works a little better @jamesbiggs?

@jamesbiggs
Copy link
Collaborator

Hey AJ, I've just tried fab build && fab start and just fab start on its own, but I'm getting this error in the web container now:

2023-07-19 11:16:12 chmod: cannot access 'bash/dev-watch.sh'$'\r': No such file or directory
: not found11:16:12 bash/run-dev.sh: 3: 

Which just keeps running in a loop. I've checked and I've definitely got the latest version of this branch, and I can even see dev-watch.sh 🤔

@ahosgood
Copy link
Member Author

You can now add the following to your .env to run migrations and start the server automatically:

AUTO_START_SERVER=true

@ahosgood
Copy link
Member Author

@jamesbiggs - Really sorry to have to ask but when you get a chance, could you try again? I'm hoping I've squashed the issue you were having with chmod: cannot access 'bash/dev-watch.sh'... 🤞🏼

@jamesbiggs
Copy link
Collaborator

Hey @ahosgood, I've given it a go and I'm getting this now:

2023-07-20 13:15:13 chmod: cannot access 'bash/dev-watch.sh'$'\r': No such file or directory
: not found13:15:13 /app/bash/run-dev.sh: 3: 
2023-07-20 13:15:13 /app/bash/run-dev.sh: 10: Syntax error: end of file unexpected (expecting "then")

I think it might be this $'\r' it's looking for. I had a look and found a few SO threads that are talking about this and Windows, and how it gives a different format or something... So I'm wondering if these are of any use..?
https://stackoverflow.com/a/22828389
https://stackoverflow.com/a/56404890

@ahosgood ahosgood removed the request for review from Puththiran July 20, 2023 14:43
@jamesbiggs
Copy link
Collaborator

jamesbiggs commented Jul 20, 2023

Hi @ahosgood, I ran some commands on this branch and compared the times to develop:

develop

fab start

real 1m46.300s
user 0m0.015s
sys 0m0.062s

fab sh

I had to type exit to get the time, so it's slightly inaccurate
real 0m1.967s
user 0m0.000s
sys 0m0.015s

fab pull-staging-data

real 0m52.494s
user 0m0.015s
sys 0m0.000s

fab pull-staging-media

real 0m7.344s
user 0m0.015s
sys 0m0.000s

createsuperuser

I had to enter credentials to retrieve time
real 0m12.282s
user 0m5.194s
sys 0m0.619s

makemigrations

real 0m10.057s
user 0m5.024s
sys 0m0.509s

migrate

real 0m11.110s
user 0m5.590s
sys 0m0.843s

runserver

I had to kill server as soon as it finished running
real 0m14.877s
user 0m2.817s
sys 0m0.278s

aws-migration

fab start

real 1m5.080s
user 0m0.000s
sys 0m0.015s

fab sh

I had to type exit to get the time, so it's slightly inaccurate
real 0m4.425s
user 0m0.031s
sys 0m0.016s

fab pull-staging-data

real 1m0.049s
user 0m0.047s
sys 0m0.000s

fab pull-staging-media

real 0m9.517s
user 0m0.015s
sys 0m0.031s

createsuperuser

I had to enter credentials to retrieve time
real 0m15.720s
user 0m5.563s
sys 0m0.589s

makemigrations

real 0m10.479s
user 0m5.420s
sys 0m0.568s

migrate

real 0m11.682s
user 0m6.126s
sys 0m0.728s

runserver

I had to kill server as soon as it finished running
real 0m15.060s
user 0m3.232s
sys 0m0.148s

Seems like the time to start the server is quicker on this branch, but everything else is slightly slower (only by a few seconds in most cases)

I'm just going to restart my laptop though as I've found before that restarting my laptop makes things start quicker, and any time I rebuild things my laptop gets slower and slower...

@jamesbiggs
Copy link
Collaborator

After restarting PC, on this branch:

fab start

real 0m54.140s
user 0m0.015s
sys 0m0.062s

makemigrations

real 0m8.844s
user 0m4.534s
sys 0m0.443s

and on develop:

fab start

real 1m24.347s
user 0m0.000s
sys 0m0.062s

makemigrations

real 0m14.645s
user 0m6.439s
sys 0m0.432s

So looks like this one is running slightly faster now 🙂

I've also noticed something, not sure if it holds any relevance...

When I run fab sh, on develop I get this:
root@1c80d429a74f:/app#

but on this branch I get:
app@ff5e7d2ac3b7:/app$

I'm not sure if that's intended but just something I noticed 😃🙌

@ahosgood
Copy link
Member Author

When I run fab sh, on develop I get this: root@1c80d429a74f:/app#

but on this branch I get: app@ff5e7d2ac3b7:/app$

This is now down to the fact that the Docker image is no longer running as the root user who would get the prompt of #.

We now have a non-root/standard user (app) who would get the prompt of $.

@jamesbiggs jamesbiggs self-requested a review July 21, 2023 12:27
Copy link
Collaborator

@jamesbiggs jamesbiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work AJ, and thanks for answering all the questions! 🙌😃

Copy link
Collaborator

@JohnHeeryTNA JohnHeeryTNA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving from FE workflow perspective.

@ahosgood ahosgood merged commit dc39c05 into develop Jul 24, 2023
5 checks passed
@ahosgood ahosgood deleted the feature/aws-migration branch July 24, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants