Replace Travis CI with GitHub actions#1086
Conversation
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| php-version: [ '5.6', '7.4', '8.3' ] |
There was a problem hiding this comment.
note: PHP 5.6 is still marked as supported on the wordpress.org plugin page so I think it makes sense to test it.
Testing 8.3 as well because it's currently the latest PHP version officially supported by WP.
Included 7.4 as well so we can confirm it works on the latest minor version of PHP 7. Arguably this one could be skipped?
There was a problem hiding this comment.
Adding 8.3 is great, let us keep the other versions for now. We might reduce them at some point.
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '16' |
There was a problem hiding this comment.
note: Using Node 16 as this is the version specific in the .nvmrc file. We might want to consider upgrading it as it's quite an old version, but that's probably out of the scope for this task.
There was a problem hiding this comment.
Yes, we have a separate task to upgrade that.
| * @return array | ||
| */ | ||
| public function default_image_transformations( $default ) { | ||
| public function default_image_transformations( $default ) { // phpcs:ignore Universal.NamingConventions.NoReservedKeywordParameterNames.defaultFound |
There was a problem hiding this comment.
note: We could consider renaming all the variables using a reserved keyword... But I'm always a bit wary to do these type of refactor without properly testing (things could easily go wrong if the IDE misses something like a string interpolation, or dynamic calling the variable perhaps). And testing all of these could take a significant amount of time.
There was a problem hiding this comment.
I agree, let us keep the previous names
| <!-- Include WP VIP coding standard checks --> | ||
| <rule ref="WordPress-VIP-Go" /> | ||
| <rule ref="WordPress-VIP-Go"> | ||
| <exclude name="WordPressVIPMinimum.Functions.RestrictedFunctions.wp_remote_get_wp_remote_get" /> |
There was a problem hiding this comment.
note: There was an error about using wp_remote_get, recommending to use the VIP function instead... but if the plugin isn't used on VIP, that function won't be available! So IMO it makes more sense to ignore that specific rule globally.
| "react-hooks/rules-of-hooks": "error", | ||
| "react-hooks/exhaustive-deps": "warn" | ||
| "react-hooks/exhaustive-deps": "warn", | ||
| "@wordpress/no-global-event-listener": "off" |
There was a problem hiding this comment.
note: This rule was showing several warnings in our JS. However it is really only needed in the context of a React application (see here) so I believe it can be disabled
Doing only on push should be enough and avoid duplication of workflows
gabrielcld2
requested review from
aatanasov-cloudinary
and removed request for
aleksandar-cloudinary
aatanasov-cloudinary
left a comment
aatanasov-cloudinary
left a comment
There was a problem hiding this comment.
@gabrielcld2, looks good!
3 participants
Approach
QA notes