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

Implement publishing workflow for standalone plugins that aren't modules #1000

Merged
merged 12 commits into from
Feb 27, 2024

Conversation

mukeshpanchal27
Copy link
Member

Summary

Fixes #935

The PR contains the following changes:

  • Updated the plugins.json file to support the plugins config
  • Introduced the get-plugin-dir command to retrieve the plugin directory, either build or plugins
  • Updated get-plugin-version to allow returning plugin versions
  • Updated test-plugins command to also test the plugins
  • Updated deploy-standalone-plugins.yml workflow to release the plugins

Some notes:

  • Added pull_request for checking automated workflow
  • Set both auto and manual workflow DRY Run to true for security
  • Updated SVN credentials for double security

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release Creating standalone plugins labels Feb 20, 2024
@mukeshpanchal27 mukeshpanchal27 self-assigned this Feb 20, 2024
@mukeshpanchal27
Copy link
Member Author

@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review February 20, 2024 06:07
Copy link

github-actions bot commented Feb 20, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

plugins.json Outdated
"plugins": {
"auto-sizes": {
"slug": "auto-sizes",
"version": "1.0.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw that comment. Still, not ideal to have the version in triplicate. If you want to address that in another issue, sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open a separate issue for further work: #1006

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bin/plugin/commands/test-plugins.js Outdated Show resolved Hide resolved
bin/plugin/commands/get-plugin-dir.js Outdated Show resolved Hide resolved
bin/plugin/commands/get-plugin-dir.js Outdated Show resolved Hide resolved
bin/plugin/commands/get-plugin-dir.js Outdated Show resolved Hide resolved
bin/plugin/commands/get-plugin-dir.js Outdated Show resolved Hide resolved
bin/plugin/commands/get-plugin-dir.js Outdated Show resolved Hide resolved
bin/plugin/commands/get-plugin-version.js Outdated Show resolved Hide resolved
bin/plugin/commands/get-plugin-version.js Outdated Show resolved Hide resolved
bin/plugin/commands/test-plugins.js Outdated Show resolved Hide resolved
plugins.json Outdated
"plugins": {
"auto-sizes": {
"slug": "auto-sizes",
"version": "1.0.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw that comment. Still, not ideal to have the version in triplicate. If you want to address that in another issue, sure.

@mukeshpanchal27
Copy link
Member Author

Thanks, @westonruter, for the feedback. I've addressed all of it. The PR is now ready for review.

@westonruter westonruter mentioned this pull request Feb 22, 2024
3 tasks
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a bit of feedback, but want to give this another look with fresh eyes and do some local testing.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple trivial suggestions not blocking merge, but otherwise LGTM.

bin/plugin/commands/get-plugin-dir.js Outdated Show resolved Hide resolved
bin/plugin/commands/get-plugin-dir.js Outdated Show resolved Hide resolved
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mukeshpanchal27 This looks solid to me, except for two things:

  • The change of including the plugin versions in plugins.json is unnecessary, based on the other feedback. The way the file is structured in feature/modules-to-plugins is correct.
  • Per https://github.com/WordPress/performance/pull/1000/files#r1503461064, let's double check that the .wordpress-org directories of the plugins are excluded from the ZIP builds. We probably just have to add an according directive to the .gitattributes file for that to work.

plugins.json Outdated
"plugins": {
"auto-sizes": {
"slug": "auto-sizes",
"version": "1.0.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@mukeshpanchal27
Copy link
Member Author

Thanks @felixarntz @westonruter @swissspidy @joemcgill for the review. I've made updates based on the suggestions and discussions:

  • Removed versions from the plugins in the plugins.json file.
  • Extended the version script to read versions from readme.txt.
  • Excluded .wordpress-org for plugins.
  • Updated docblocks according to the suggestions.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @mukeshpanchal27!

@felixarntz felixarntz merged commit c42da44 into feature/modules-to-plugins Feb 27, 2024
14 checks passed
@felixarntz felixarntz deleted the fix/935-update-workflow branch February 27, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants