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

Improve Multi-Instance Docker Support and DB Restoration Fix #47

Merged

Conversation

gabepetersen
Copy link
Contributor

This PR will accomplish two tasks:

  1. The ability for multiple Optimizely Docker projects to be created without storage conflicts in /var/opt/mssql/host_data

    • This commit 7a2b9b5 will add a .env file to each Docker project that specifies a DB_DIRECTORY. This environment variable will specify a new directory on /var/opt/mssql/host_data/ within the container so that the DB may persist when adding other docker projects on the base image of mssql/server:2019-latest.
  2. The ability to restore pre-existing databases from .mdf & .ldf files on docker compose up

    • If the container and its network are deleted, then the databases will be dropped. So when docker compose up runs again to create the containers, then the plain CREATE DATABASE command won't work since there is already a .mdf & .ldf file copied over. This commit 3ca4fc2 will check if the .mdf file exists. If so, it will add a FOR ATTACH command to the end of CREATE DATABASE to restore the DB from .mdf and .ldf.

This will add a .env file to each Docker project that specifies a DB_DIRECTORY. This environment variable will specify a new directory on /var/opt/mssql/host_data/ within the container so that the DB may persist when adding other docker projects of type mssql/server:2019-latest.
If the container and its network are deleted, then the databases will be
dropped. So when `docker compose up` runs again to create the
containers, then the `CREATE DATABASE` command won't work since there is
already a .mdf/.ldf file existent. This is why the `FOR ATTACH` command
is needed in this case if the files already exist.
gabepetersen added a commit to gabepetersen/OptiGraphExperiment that referenced this pull request Oct 25, 2023
@JohanPetersson
Copy link
Contributor

Thank you for this contribution. Before we can merge this, we need make this work a bit better with the templating system;

SA_PASSWORD=Qwerty12345!
DB_NAME=Alloy.Mvc.1
DB_DIRECTORY=Alloy.Mvc.1

@gabepetersen
Copy link
Contributor Author

@JohanPetersson I apologize for my late response to this. The recent commit 182b656 should update the code according to your recent comment. For the empty commerce template .env file, I added this to distinguish the two databases:

...
DB_NAME=Commerce.Empty.1_cms
DB_NAME_COMMERCE=Commerce.Empty.1_commerce
...

Copy link
Contributor

@JohanPetersson JohanPetersson left a comment

Choose a reason for hiding this comment

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

I've tested all templates now. Everything looks fine when the proposed changes I added now are in place. Thank you so much!

templates/Commerce.Empty/.env Outdated Show resolved Hide resolved
templates/Alloy.Mvc/Docker/create-db.sh Outdated Show resolved Hide resolved
This will also fix the DB directory for the empty commerce template to
be the variable of the project name
@gabepetersen
Copy link
Contributor Author

@JohanPetersson Sorry again for the late response, I was out on vacation. Fixes to the recent change requests are in 4a9d57c. Since I accidentally removed the newlines from the .env files instead of the .sh files in a commit I added, I had to force push a replace commit.

JohanPetersson
JohanPetersson previously approved these changes Nov 13, 2023
Copy link
Contributor

@JohanPetersson JohanPetersson left a comment

Choose a reason for hiding this comment

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

Thank you so much again for your contribution!

@JohanPetersson JohanPetersson merged commit 0cd218e into episerver:main Nov 14, 2023
1 check passed
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.

2 participants