Skip to content
This repository was archived by the owner on Nov 10, 2025. It is now read-only.

Make "db/" into a volume inside the Docker container#769

Merged
AP-Hunt merged 1 commit into
mainfrom
enable_db_mirgations_with_ro_fs
May 28, 2025
Merged

Make "db/" into a volume inside the Docker container#769
AP-Hunt merged 1 commit into
mainfrom
enable_db_mirgations_with_ro_fs

Conversation

@AP-Hunt
Copy link
Copy Markdown
Contributor

@AP-Hunt AP-Hunt commented May 28, 2025

What problem does this pull request solve?

Trello card: https://trello.com/c/KoHvaEUA/681-aws-m112-ecs-read-only-root-filesystem-configuration

AWS ECS docs [1] say that a file that exists in a directory that is subsequently marked as a volume will be copied to the host and remounted into the container.

This is necessary for us to do with the "db/" directory because the "db:migrate" Rake task writes to "db/schema.rb"

[1] https://docs.aws.amazon.com/AmazonECS/latest/developerguide/bind-mounts.html

Things to consider when reviewing

  • Ensure that you consider the wider context.
  • Does it work when run on your machine?
  • Is it clear what the code is doing?
  • Do the commit messages explain why the changes were made?
  • Are there all the unit tests needed?
  • Has all relevant documentation been updated?

AWS ECS docs [1] say that a file that exists in a directory that is
subsequently marked as a volume will be copied to the host and remounted into
the container.

This is necessary for us to do with the "db/" directory because the
"db:migrate" Rake task writes to "db/schema.rb"

[1] https://docs.aws.amazon.com/AmazonECS/latest/developerguide/bind-mounts.html
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@lfdebrux lfdebrux left a comment

Choose a reason for hiding this comment

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

This is necessary for us to do with the "db/" directory because the "db:migrate" Rake task writes to "db/schema.rb"

That's surprising, I would have thought that db:migrate would write to db/schema.rb only if there are any changes 🤔

Oh well

@AP-Hunt
Copy link
Copy Markdown
Contributor Author

AP-Hunt commented May 28, 2025

only if there are any changes

That is true. We saw this on forms-runner and it caused an error. We're porting the fix over here to stop that happening.

@lfdebrux
Copy link
Copy Markdown
Contributor

lfdebrux commented May 28, 2025

only if there are any changes

That is true. We saw this on forms-runner and it caused an error. We're porting the fix over here to stop that happening.

Hmm. Ideally there shouldn't be changes to db/schema.rb from running db:migrate, as that indicates a configuration drift. Ideally any changes would have been committed to the main branch before the deploy.

@AP-Hunt
Copy link
Copy Markdown
Contributor Author

AP-Hunt commented May 28, 2025

Hmm. Ideally there shouldn't be changes to db/schema.rb from running db:migrate, as that indicates a configuration drift. Ideally any changes would have been committed to the main branch before the deploy.

That's .. concerning? Probably? We were seeing this in staging, and in review once we began running db:migrate and not just db:setup: govuk-forms/forms-runner@7d943ce

@lfdebrux
Copy link
Copy Markdown
Contributor

Hmm. Ideally there shouldn't be changes to db/schema.rb from running db:migrate, as that indicates a configuration drift. Ideally any changes would have been committed to the main branch before the deploy.

That's .. concerning? Probably? We were seeing this in staging, and in review once we began running db:migrate and not just db:setup: alphagov/forms-runner@7d943ce

Yeah I saw that. It is concerning, but only slightly? Probably not something that needs to be fixed right this moment.

@AP-Hunt AP-Hunt merged commit df9685d into main May 28, 2025
5 checks passed
@AP-Hunt AP-Hunt deleted the enable_db_mirgations_with_ro_fs branch May 28, 2025 13:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants