-
Notifications
You must be signed in to change notification settings - Fork 29
feat(db): to add resources to import bacpac snapshots to db #26
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi Carlos, |
| - SA_PASSWORD=localSAPassw0rd | ||
| depends_on: | ||
| - mssql | ||
| mssqlimport: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please combine / merge mssqlimport into the exisitng mssqlinit container.
There should be one container and script that either imports the bacpac (if present) or bootstraps a fresh DB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @mpern
I can merge both mssqlimport and the mssqlinit into the same script. As a bacpac file is not a backup but a snapshot we need an already created database to import the content of the snapshot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right!
I should have looked at your script more carefully.
In that case:
Always create db
Import bacpac, if file is available (and not imported previously)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @mpern . I'm progressing on the requested changes. After testing, I've found that it is possible to import a bacpac without an already created database. Despite to that, I think it would be better to initialise brand new database anyway and then check if a bacpac is present to be imported. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not vouching the idea of having the init and the import on the same docker-compose service.
I'm keeping both services apart for now. I will wait for your review for the changes I've just added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point.
Let's keep using two different services for now.
docker-resources/import-db.sh
Outdated
| #https://learn.microsoft.com/en-us/sql/tools/sqlpackage/sqlpackage-download?view=sql-server-ver16 | ||
| apt-get update && apt-get install -y unzip libunwind8 libicu55 | ||
|
|
||
| cd ~ | ||
| curl -s -L -o sqlpackage.zip https://aka.ms/sqlpackage-linux | ||
| mkdir ~/sqlpackage | ||
| unzip sqlpackage.zip -d ~/sqlpackage | ||
| echo "export PATH=\"\$PATH:$HOME/sqlpackage\"" >> ~/.bashrc | ||
| chmod a+x ~/sqlpackage/sqlpackage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move install steps to a Dockerfile and use docker compose build
e.g.
...
mssqlinit:
build: ./docker-resources/init
volumes:
- ./docker-resources:/tmp/resources
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello. This change has been added. The unique way of getting it done was to name the file Dockerfile and point to the docker-resources folder as the folder to find the build file for docker.
docker-resources/import-db.sh
Outdated
|
|
||
| echo "SQL Server is up; Starting bacpac import..." | ||
| # Run the setup script to create the DB and the schema in the DB | ||
| /root/sqlpackage/sqlpackage /a:Import /sf:/tmp/resources/backup.bacpac /TargetServerName:mssql /TargetDatabaseName:localdev /TargetUser:sa /TargetPassword:$SA_PASSWORD No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if file backup.bacpac exists before executing the import
nice to have:
after import, touch a kind of marker file to indicate that the file has already been imported; skip import if marker is present.
if you want to be really fancy, you could calculate a hash of the bacpac and use that to determine if you need to run the import again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello.
I have checked what happens when I try to import twice the snapshot. I get the following error:
Importing to database 'localdev' on server 'mssql'.
Creating deployment plan
Initializing deployment
*** Error importing database:Data cannot be imported into target because it contains one or more user objects. Import should be performed against a new, empty database.
Error SQL0: Data cannot be imported into target because it contains one or more user objects. Import should be performed against a new, empty database.
It seems that once there is data on the database, we can not import any other snapshot.
What are your thoughts on this?
feat: to add condition to import the snapshot fix: to rename db backup to db snapshot feat: to add .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more things to adapt, but nothing major
| if [ $UBUNTU_VERSION -eq 16 ]; then apt-get install -y libicu55; elif [ $UBUNTU_VERSION -eq 17 ]; then apt-get install -y libicu57; \ | ||
| elif [ $UBUNTU_VERSION -eq 18 ]; then apt-get install -y libicu60; elif [ $UBUNTU_VERSION -eq 20 ]; then apt-get install -y libicu66; fi && \ | ||
| curl -s -L -o /opt/sqlpackage.zip https://aka.ms/sqlpackage-linux && mkdir /opt/sqlpackage && unzip /opt/sqlpackage.zip -d /opt/sqlpackage && \ | ||
| rm -f /opt/sqlpackage.zip && chmod a+x /opt/sqlpackage/sqlpackage && echo "export PATH=\"\$PATH:$/opt/sqlpackage\"" >> ~/.bashrc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of updating PATH in bashrc, use ENV
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#env
| @@ -0,0 +1 @@ | |||
| *.bacpac No newline at end of file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That reminds me:
Please create a .dockerignore for docker-resources to ignore bacpac files while building the import container
OR
move the dockerfile of mssqlimport to a different subfolder
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#exclude-with-dockerignore
| - SA_PASSWORD=localSAPassw0rd | ||
| depends_on: | ||
| - mssql | ||
| mssqlimport: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point.
Let's keep using two different services for now.
This PR adds capabilities to import bacpac snapshots using the mssqlimport service