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

Force install of specific WordPress version #28

Merged

Conversation

@mmcachran
Copy link
Contributor

@mmcachran mmcachran commented Oct 18, 2019

Check if wp_version in configuration and upgrade/downgrade the site if set regardless of if the site is already installed.

Copy link
Member

@tomjn tomjn left a comment

Thanks for the PR :) Are there any potentials for data loss with this? I'm wondering if it's better to add a force_wp parameter, and then warn the user if the WP version they chose and the WP version they have are different? Anything we can do to make things more informative for the user and set expectations as to how things will work is great

Given that the default value of WP_VERSION is defined at the top as latest, a skim read would suggest this means that the latest version of WP gets reinstalled over the top on every provision

@mmcachran
Copy link
Contributor Author

@mmcachran mmcachran commented Oct 18, 2019

There shouldn't be any potential for data loss, but good call on the unnecessary call since WP_VERSION defaults to "latest". My hunch is that wp core download would short circuit itself if the version installed is the same as the version trying to be downloaded, but I'll do some testing to ensure this is the case. I'll also add an additional parameter for force_wp.

@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 18, 2019

And don't forget to update the readme :-D

README.md Outdated Show resolved Hide resolved
@Mte90 Mte90 requested a review from tomjn Oct 18, 2019
Copy link
Member

@Mte90 Mte90 left a comment

For me is ok, waiting for a review from @tomjn

@mmcachran mmcachran requested a review from Mte90 Oct 18, 2019
provision/vvv-init.sh Outdated Show resolved Hide resolved
@mmcachran mmcachran requested a review from Mte90 Oct 18, 2019
@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 18, 2019

I am wondering what is the difference with https://github.com/Varying-Vagrant-Vagrants/custom-site-template/blob/master/provision/vvv-init.sh#L103

GitHub
For when you just need a simple dev site. Contribute to Varying-Vagrant-Vagrants/custom-site-template development by creating an account on GitHub.

@mmcachran
Copy link
Contributor Author

@mmcachran mmcachran commented Oct 18, 2019

I think the difference is being able to downgrade to a specific version of WP if needed. For example, if something is only breaking in one version of Core (the version used on production and the client doesn't want to update for some reason or is roadblocked by testing) the user could just update their config and reprovision instead of having to ssh into the VM. If it's better to just move the logic up I'm happy to update the PR 🙂

@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 18, 2019

I am only wondering if it is a duplicate code because as I can see will be executed first the update (with a specific version) if the website is installed at https://github.com/Varying-Vagrant-Vagrants/custom-site-template/blob/master/provision/vvv-init.sh#L103 and later again.

So maybe is more simple to check in that line what is the wp version (wpcli can already do it without the bash snippet) and check if wp-version is different (configured in the yaml) and install it.
So we can remove the new parameter saw before.

Sorry for this analysis now but it was a busy day today and I didn't had time to analyze everything carefully.

GitHub
For when you just need a simple dev site. Contribute to Varying-Vagrant-Vagrants/custom-site-template development by creating an account on GitHub.

@mmcachran
Copy link
Contributor Author

@mmcachran mmcachran commented Oct 18, 2019

Good point, @Mte90! That approach sounds much cleaner. I've updated the PR to reflect this.

@Mte90 Mte90 changed the title Feature/wp version updates Force install of specific WordPress version Oct 21, 2019
Mte90
Mte90 approved these changes Oct 21, 2019
@Mte90 Mte90 self-requested a review Oct 21, 2019
@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 21, 2019

Sorry I checked again the code and I discovered that now in WP-CLI core is not possible to define what WP version to install with wp core install but the code of wpcli include the support to download from different version.

The issue is that we are installing on first provision, at second we downgrade with the update command, so is better to install wp already with the right version to avoid a second provision.

@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 21, 2019

Also we are downloading the wp version https://github.com/Varying-Vagrant-Vagrants/custom-site-template/blob/master/provision/vvv-init.sh#L38 specified in the config.
So seems that the problem of this pr to recover is to downgrade an already preset installation to a different wp version.

GitHub
For when you just need a simple dev site. Contribute to Varying-Vagrant-Vagrants/custom-site-template development by creating an account on GitHub.

Mte90
Mte90 approved these changes Oct 21, 2019
@Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 21, 2019

And nevermind it was right as it is now, it' only me that need to learn to be more focused during reviews...

@Mte90 Mte90 merged commit 62f69d1 into Varying-Vagrant-Vagrants:master Oct 21, 2019
@mmcachran
Copy link
Contributor Author

@mmcachran mmcachran commented Oct 21, 2019

Apologies, @Mte90! I was wrapped up in client stuff all day and was just able to check on this PR. Thanks for the review and merging :)

@mmcachran mmcachran deleted the feature/wp-version-updates branch Oct 21, 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