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

Update tested up to label and test matrix #155

Merged
merged 4 commits into from
Jan 4, 2024
Merged

Conversation

ouikhuan
Copy link
Contributor

  • This PR test for WordPress 6.4.2 with PHP 7.0, 7.4 and 8.0.
  • Since latest WordPress is not supporting PHP 5.6 anymore, we also need to modify test.yml

How to test:

  • Clone the plugin under your working directory
  • Using wp-env to test, if you haven't installed it, please visit wp-env for instructions
  • It'll be easier if you have a .wp-env.json file at the root folder. For example, if your root folder is oss-importer and you can clone the plugin under this folder. Create .wp.-env.json file, you can change PHP version to the one you want to test with.
{
  "phpVersion": "7.0",
  "plugins": ["./wordpress-importer"]
}
  • Run wp-env start to spin up the WordPress
  • Navigate to http://localhost:8888/wp-admin/admin.php?import=wordpress and perform an import
  • Import the content; it can take a while if you have a lot of content. If the script timeout, you need to set_time_limit to a higher value
  • Check and see if the import works fine without errors

@ouikhuan ouikhuan self-assigned this Dec 27, 2023
@ouikhuan
Copy link
Contributor Author

I realized that even after removing PHP 5.6 from the test.yml matrix it's still showing up as expected check. Should we do this update in another PR?

@jrfnl
Copy link
Member

jrfnl commented Dec 27, 2023

I realized that even after removing PHP 5.6 from the test.yml matrix it's still showing up as expected check. Should we do this update in another PR?

That is unrelated to your PR. The required builds are defined in the protected branch settings and the way the branch protection is set up for this repo it will now literally block every single PR from being merged.

I just checked and while I can commit, I cannot change the branch protection, so you now have the not very enviable task to figure out who on this earth has admin rights for this repo and can set the branch protection to something a little more sane.

In my opinion, the following changes need to be made:

  • The required checks need to be updated to be in sync with the current "non-experimental" builds. Quite a few are missing (and for this PR, one needs to be removed).
  • The "Require branches to be up to date before merging " check needs to be removed as it is completely silly to require a rebase of all open PRs after each merge.
  • The "Do not allow bypassing the above settings" check should be removed to allow select people to merge PRs even when the builds don't pass. This ability should generally only be used with discretion and careful after careful evaluation, but there are circumstances where multiple PRs need to be merged before a passing build can be achieved again and it is now impossible to get out of that type of situation.

Aside from the above, there are a number of PRs open (and rotting for over a year) which make various improvements to the GH Actions workflows. Those PRs should be merged with priority to keep things running smoothly and never be left to rot.

I would merge them myself now, except due to the long time between when I pulled them and now and having to rebase them due to the branch protection settings, I can't merge them anymore with the failing PHP 5.6 build.

In my opinion, we should also add a dependabot config to automatically update action runners as those are going out of date fast now too.

All in all, it comes down to nobody taking ownership of this repo and that being problematic when changes need to be made/merged/released.

@ouikhuan
Copy link
Contributor Author

Thanks for the suggestion @jrfnl , please let me know if I can help on those pending PRs from you. @vishnugopal, what do you think based on the suggestion above? Do we have permission to modify the settings?

@dd32 dd32 merged commit 5d081c8 into master Jan 4, 2024
22 checks passed
@dd32 dd32 deleted the update/tested-up-to-label branch January 4, 2024 01:58
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.

3 participants