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

Check if the site is installed, and then attempt to restore from backup #25

Conversation

@msaggiorato
Copy link
Member

@msaggiorato msaggiorato commented Oct 12, 2019

Potentially fix an issue when reprovisioning without restoring, which would install the database over and over again with the one from the backup.

This issue was introduced on #23.

On that PR @tomjn mentioned that the db_restore script already does this, and it's true tho I personally think this is a more elegant solution, which only restores the sites to be provisioned, whereas the restore script restores every sql file found on the backups directory.

If we agree on that, the restore script could be removed as this would make it unnecessary.

@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 12, 2019

db_restore can be used do to that process automatically.
I know that a lot of people share the db on cloud sync so maybe they are interested on update manually. Maybe we can move that code in db_restore and call it from there to avoid code duplication.

@msaggiorato
Copy link
Member Author

@msaggiorato msaggiorato commented Oct 12, 2019

But isn't restoring all databases (even the ones you don't need on the VM), a not so clean solution?

In my experience, some databases are sometimes huge, and you only need them while you are working on that particular site. I'd rather not have this database loaded or provisioned when I don't need it.

The backup can still exist for sync workflows on the backup directory, it just won't be loaded in your VM if you don't need it.

Another topic, restoring the database from a completely separate script (located in another directory) from the site provisioner is keeping the code scattered around. So even if we cannot agree on my argument and move it here to the site provisioner, this should be moved to the provision folder at the very least.

@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 12, 2019

For me the idea of this script is good, I am wondering about the db_restore to script if we want to have duplicate code to do the same things, that's it :-)

@tomjn
Copy link
Member

@tomjn tomjn commented Oct 13, 2019

@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 14, 2019

So we want to approve that pr and merge it?

@msaggiorato
Copy link
Member Author

@msaggiorato msaggiorato commented Oct 15, 2019

Considering arbitrary databases makes me think that those should be handled by a custom site provisioner.

Backing up is definetely good to be handled by VVV core, since it's nice to have a save process that saves everything. But when restoring (either from backup, or within the site folder), I think it's useful to leave it up to the site provisioners. If multiple databases are needed, forking the site provisioner and adding the extra databases should suffice.

Another useful case for this is using vagrant provision --provision-with=site-xxxx or vagrant reload --provision-with=site-xxxx command, which might be a nice way to provision a new site without running the full provisioner (updating packages and such, which is not necessary all of the times). At the current state this is not working properly, because some things are not running (like the SSL certificates generation), but I think this is a good step towards this use.

@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 15, 2019

Maybe we can implement an alert about arbitrary database? Or create a documentation page for it

@tomjn
Copy link
Member

@tomjn tomjn commented Oct 15, 2019

@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 15, 2019

So is better to document the case with an arbitrary sql dump and move on this pr.

tomjn
tomjn approved these changes Oct 28, 2019
@tomjn tomjn merged commit 2624300 into Varying-Vagrant-Vagrants:master Oct 28, 2019
tomjn added a commit that referenced this issue Oct 28, 2019
Fix critical bug after merging #25 on top of #28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants