Implement publishing workflow for standalone plugins that aren't modules#1000
Conversation
|
Automated deploy working fine: https://github.com/WordPress/performance/actions/runs/7969138800/job/21754473267?pr=1000 |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| "plugins": { | ||
| "auto-sizes": { | ||
| "slug": "auto-sizes", | ||
| "version": "1.0.1" |
There was a problem hiding this comment.
Should this be necessary if the version is already present here:
Should this not rather parse the version out of the plugin file? Or we also have it present in the readme.txt:
I think we should avoid having this version in three different places.
There was a problem hiding this comment.
If we just store the version in Stable Tag of readme.txt then the plugin file could have n.e.x.t as the version which gets replaced during deployment, or vice-versa.
There was a problem hiding this comment.
Or the version could be stored in plugins.json like you have here, but if so, then n.e.x.t should be the version in auto-sizes.php and readme.txt to avoid having to manually keep them in sync.
There was a problem hiding this comment.
Like the idea of storing the version in a single place only. As mentioned n.e.x.t strategy can be used to populate the version on the fly.
There was a problem hiding this comment.
Thanks, @westonruter, for bringing this up. The plugin versions were added for the deploy process per #935 (comment). Since there is no build project for plugins, in my opinion, we should discuss this first and address it in a follow-up issue. What do you think? cc. @joemcgill
There was a problem hiding this comment.
Yes, I saw that comment. Still, not ideal to have the version in triplicate. If you want to address that in another issue, sure.
There was a problem hiding this comment.
Open a separate issue for further work: #1006
There was a problem hiding this comment.
I spent some time looking into how we could modify the release job to avoid the duplication of version numbers here for this, and I'm not sure it's worth the effort for now. Once we finish migrating all of the standalone modules to the plugins folder we can clean this up more easily.
There was a problem hiding this comment.
@mukeshpanchal27 @joemcgill I just voiced a similar concern as @westonruter in #935 (comment), and his feedback is precisely why the requirements only mentioned plugin slugs (no need to duplicate the version number).
I'm okay for this to be addressed in a separate issue, but since this PR is actually making the change and the feature/modules-to-plugins branch currently uses the correct format, it's a little strange IMO to go ahead with this change just to change it back afterwards.
If this PR is blocking other work and therefore would benefit from a merge ASAP, I'm okay to fix this again in a follow up issue, but it should be addressed shortly after this PR, in time for 3.0, as it requires changes to the plugins.json file format/spec, which will affect documentation.
There was a problem hiding this comment.
Thanks for the feedback.
In a4cf3bf, I have reverted the version-related changes for plugins and instead fetched the version from the readme.txt file. I updated the script accordingly. I opted not to use the plugin's main file because different plugins have different main files, such as plugin/load.php or plugin/plugin.php.
…mance into fix/935-update-workflow
| "plugins": { | ||
| "auto-sizes": { | ||
| "slug": "auto-sizes", | ||
| "version": "1.0.1" |
There was a problem hiding this comment.
Yes, I saw that comment. Still, not ideal to have the version in triplicate. If you want to address that in another issue, sure.
|
Thanks, @westonruter, for the feedback. I've addressed all of it. The PR is now ready for review. |
joemcgill
left a comment
There was a problem hiding this comment.
Left a bit of feedback, but want to give this another look with fresh eyes and do some local testing.
westonruter
left a comment
There was a problem hiding this comment.
Just a couple trivial suggestions not blocking merge, but otherwise LGTM.
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 This looks solid to me, except for two things:
- The change of including the plugin versions in
plugins.jsonis unnecessary, based on the other feedback. The way the file is structured infeature/modules-to-pluginsis correct. - Per https://github.com/WordPress/performance/pull/1000/files#r1503461064, let's double check that the
.wordpress-orgdirectories of the plugins are excluded from the ZIP builds. We probably just have to add an according directive to the.gitattributesfile for that to work.
| "plugins": { | ||
| "auto-sizes": { | ||
| "slug": "auto-sizes", | ||
| "version": "1.0.1" |
There was a problem hiding this comment.
@mukeshpanchal27 @joemcgill I just voiced a similar concern as @westonruter in #935 (comment), and his feedback is precisely why the requirements only mentioned plugin slugs (no need to duplicate the version number).
I'm okay for this to be addressed in a separate issue, but since this PR is actually making the change and the feature/modules-to-plugins branch currently uses the correct format, it's a little strange IMO to go ahead with this change just to change it back afterwards.
If this PR is blocking other work and therefore would benefit from a merge ASAP, I'm okay to fix this again in a follow up issue, but it should be addressed shortly after this PR, in time for 3.0, as it requires changes to the plugins.json file format/spec, which will affect documentation.
|
Thanks @felixarntz @westonruter @swissspidy @joemcgill for the review. I've made updates based on the suggestions and discussions:
|
felixarntz
left a comment
There was a problem hiding this comment.
Looks great, thanks @mukeshpanchal27!
Summary
Fixes #935
The PR contains the following changes:
plugins.jsonfile to support the plugins configget-plugin-dircommand to retrieve the plugin directory, either build or pluginsget-plugin-versionto allow returning plugin versionstest-pluginscommand to also test the pluginsdeploy-standalone-plugins.ymlworkflow to release the pluginsSome notes:
pull_requestfor checking automated workflowChecklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.