-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: install nightly version #78
fix: install nightly version #78
Conversation
Co-authored-by: Eugene Manuilov <eugene.manuilov@gmail.com>
Co-authored-by: Eugene Manuilov <eugene.manuilov@gmail.com>
Although this PR looks good from the first glance, I think we should try to use |
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.
Mostly LGTM, left the final comment.
@eugene-manuilov I moved the extract commands to a private method for better readability, can you please give this PR another review? |
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.
Thanks, @dinhtungdu! LGTM.
Is anyone actually requesting to use snapshots with the nightly WP version? |
@tlovett1 Yes, we're planning to run WPAcceptance tests against the |
Adding to @dinhtungdu's comment, we're doing this to try and proactively catch any conflicts between our plugins and core development before a core release happens and we have to react after-the-fact and with a negative user experience. |
@tlovett1 is there anything else you need to unblock merge on this PR? We're aiming for a ClassifAI release this week and this WP Snapshots enhancement will help with our WP core+ClassifAI test coverage to give us a higher level of confidence in the release, so would be ideal to get this Snapshots PR resolved early this week... thanks! |
@eugene-manuilov how do I test this? I assume a snapshot would have |
@tlovett1 @eugene-manuilov to set nightly version, I create a snapshot locally first, then update the wp version in the meta.json file to nightly. After that I push the snapshot by id to AWS. |
@dinhtungdu can you update your pull request to make sure that the version for nightly snapshots is set correctly when we run the push command? |
Updating meta.json manually to me isn't an acceptable workflow for creating a snapshot. I think this needs to be rethought a bit. Ideally, WP Snapshots would allow you somehow to set the version as |
@eugene-manuilov @tlovett1 I'm sorry for the late reply. I will add a flag to push/create command to handle the latest/nightly version. Edit: I added |
Can you give me instructions on testing this? |
@tlovett1 I add the verification steps to the PR description. |
@@ -49,6 +49,8 @@ protected function configure() { | |||
$this->addOption( 'db_name', null, InputOption::VALUE_REQUIRED, 'Database name.' ); | |||
$this->addOption( 'db_user', null, InputOption::VALUE_REQUIRED, 'Database user.' ); | |||
$this->addOption( 'db_password', null, InputOption::VALUE_REQUIRED, 'Database password.' ); | |||
|
|||
$this->addOption( 'wp_version', null, InputOption::VALUE_OPTIONAL, 'Override the WordPress version.' ); |
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.
Why is this VALUE_OPTIONAL
? You need to supply a WP version if you use this option
Currently, wpsnapshots can't install WP nightly because nightly version is available as zip file, while we only handle gz file for now. This PR fixes this issue by handling zip file for nightly version.
Verification
Create latest/nightly snapshot
(To make it clearer, please downgrade the test WP installation to one major version behind.)
cd
to a WP directory, create a snapshot with the flag--wp_version
, passinglatest
ornightly
to create snapshot with respective WP version.Pull newly created snapshot.
cd
to the WP directory, pull the snapshot created in the step above.