action-wordpress-plugin-deploy icon indicating copy to clipboard operation
action-wordpress-plugin-deploy copied to clipboard

Always include slug directory in zip

Open JasonTheAdams opened this issue 4 years ago • 12 comments

Description of the Change

When zipping a file, the zip command can zip the files with no path, a relative path, or an absolute path:

Type Zip File Result Zip Command
No Path my-plugin.php zip my-plugin.zip .
Relative Path my-plugin/my-plugin.php zip my-plugin.zip my-plugin/
Absolute Path /path/to/my-plugin/my-plugin.php zip my-plugin.zip /path/to/my-plugin

When unzipping, the path type comes into effect. No Path uses the zip file name as the directory to put the files in, or it unloads into the current directory. Relative creates a new directory with the relative structure. Absolute attempts to unload to the absolute position.

WordPress itself has a limitation when retrieving zip files from remote servers for plugin updates wherein the zip file must be in the Relative Path format. The other two will break.

This PR changes from a No Path zip to a Relative Path zip, with the plugin slug being the relative directory.

Alternate Designs

Presently, the No Path method is used when zipping. The alternatives are as outlined above: Relative and Absolute.

Benefits

When folks use the Plugins > Add New > Upload feature, if the zip files have No Path, then it assumes the zip name to be the directory for the files. The same is true when unzipping in the OS. So if a user downloads the zip, has a zip in the folder already with that name (which renames it to something like plugin.zip(1)), and then installs it, it will be installed to the plugin1 directory. By using a Relative Path, it's ensured that the directory will always reflect the plugin slug.

Possible Drawbacks

No drawbacks that I can think of. The zip file will work more intuitively.

Verification Process

We dealt with this over at GiveWP and had to wrestle this weird issue down. Our scripts now all use a Relative Path. We used this action for deployment and I noticed it's using No Path, so I thought I'd hop over and help! We've generated the zip using all the methods and tested them via upload install into WordPress.

Checklist:

  • [X] I have read the CONTRIBUTING document.
  • [X] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [ ] I have added tests to cover my change.
  • [ ] All new and existing tests passed.

Applicable Issues

Changelog Entry

Changed

  • Generated zip now stores files with relative path for WP installation consistency

JasonTheAdams avatar Jul 20 '21 21:07 JasonTheAdams

I need to basically emergency release a 2.0 to deal with a Debian image failure that means we will have something much faster moving forward anyway, so I'm sorry this will miss that release but I would like to get another one done within the next few weeks.

Could you possibly refresh this and rework it to not move the trunk directory? I don't think moving the directory matters to the job at that point but it is possible somebody is relying on it being in place in later steps, and also just kind of feels a little weird to reach in to the SVN setup like that. Thank you so much for this, I don't currently use the ZIP for anything myself so I hadn't run into these issues.

helen avatar Aug 16 '21 23:08 helen

Hi @helen!

Sure, happy to refresh this. You mention not moving the trunk directory. Do you think copying into another directory is fine? Or renaming it back to trunk after generating the zip? The latter would be much faster, if it's fine, given the repository size. One way or the other, we need a directory with all the repo contents with the repo name to be zipped up.

JasonTheAdams avatar Aug 18 '21 20:08 JasonTheAdams

@JasonTheAdams Good question - renaming trunk is probably fine so long as we name it back. I am just cautious given that somebody could potentially be doing something like adding another step in their workflow that goes in and does more in the SVN directory structure, as slim as that chance may be right now.

helen avatar Aug 25 '21 03:08 helen

So I needed to do this in a different action, and the --prefix flag seems to work for me, what do you think?

helen avatar Sep 01 '21 03:09 helen

Sorry, can you provide more context? I'm not sure what command you're referring to that supports the --prefix flag.

JasonTheAdams avatar Sep 01 '21 04:09 JasonTheAdams

Ah 🤦‍♀️ that's my fault, I forgot I was dealing with git archive instead of zip. However, in looking at it some more, it seems like making a symlink would work rather than doing a copy or move?

helen avatar Sep 01 '21 17:09 helen

My only concern with a symlink is that I'm not sure how zip would handle it on different OS's. Since this is now a composite action, we want to be a simple as possible to avoid edge cases. Rename, zip, rename back is boring but (I think) reliable.

Aiming to refresh this in the next couple days, FYI.

JasonTheAdams avatar Sep 01 '21 19:09 JasonTheAdams

It's already a little bit risky as a composite action with sed differences (and maybe even rsync), I guess somebody could be trying to do this on a Windows runner or something and I have no idea what would happen if they did. Doing a symlink seems okay to me given what's already going on in here but in reality it probably doesn't matter one way or the other unless for some reason somebody is running this on a private repo and using up their minutes, and even then it's minimal impact. No rush and no pressure, just kinda thinking out loud!

helen avatar Sep 03 '21 04:09 helen

Thanks, Helen! Honestly, I've never tried zipping up a symlink. Sounds like black magic.

That said, I just tested out zipping up a symlink, giving the resulting file a different name than the symlink, and sure enough when I unpacked it the resulting folder had the symlink name. 🤯

So when I'll go with that! Thanks for teaching me something new!

JasonTheAdams avatar Sep 03 '21 18:09 JasonTheAdams

@helen I just refreshed this. 🎉

I see in the Checklist an item for adding tests. I can't see where or how to add tests. Mind pointing me in the right direction?

Also, I suggest squashing this when all is said and done.

JasonTheAdams avatar Sep 03 '21 20:09 JasonTheAdams

Nice! There aren't any real tests besides Shellcheck for linting right now, I just approved it to run and you'll see that it's telling you to wrap something in quotes (I think one of the usages of $SLUG). Zipping a symlink def does feel like magic but I like that it works :)

I'll be taking a look at various things in the next few weeks or so to see what I want to wrap up in a new release, there's a lot of outstanding stuff around custom workspace/accommodating things like Composer better, so I want to make sure I at least know what can be done now or not. It may take me a bit simply because I have a particularly busy few weeks ahead and don't have anybody else working on maintenance here right now, just know that I very much appreciate the tip and your work here!

helen avatar Sep 04 '21 17:09 helen

Just added the double-quotes around $SLUG. I believe this is good to go.

No rush on this. I'm happy to have it added at your leisure. It's not causing us any problems. 😃

JasonTheAdams avatar Sep 09 '21 00:09 JasonTheAdams

@iamdharmesh bumping back up for your review.

jeffpaul avatar Dec 19 '22 17:12 jeffpaul