capistrano / rails

Official Ruby on Rails specific tasks for Capistrano
http://www.capistranorb.com/
MIT License
867 stars 270 forks source link

handle restore of multiple asset manifests on rollback #226

Closed lennart closed 4 years ago

lennart commented 5 years ago

Handle multiple manifest files gracefully.

Related to #123 this PR will fix issues with multiple sprockets asset manifest files lying around in #{shared_path}/public/assets/ and #{release_path}/assets_manifest_backup on cap <stage> deploy:rollback.

Currently this will fail due to multiple filepaths being interpolated into a single shell if condition:

if test "[[ -f #{source} && -f #{target} ]]"

see https://github.com/capistrano/rails/blob/master/lib/capistrano/tasks/assets.rake#L93 and https://github.com/capistrano/rails/issues/204#issuecomment-404118165

To deal with this restore_manifest will explicitly copy each of these files in a separate cp command.

Feedback on how to improve this PR is very welcome!

capistrano-bot commented 5 years ago
1 Warning
:warning: Please update CHANGELOG.md with a description of your changes. If this PR is not a user-facing change (e.g. just refactoring), you can disregard this.

Thanks for the PR! This project lacks automated tests, which makes reviewing and approving PRs somewhat difficult. Please make sure that your contribution has not broken backwards compatibility or introduced any risky changes.

Here's an example of a CHANGELOG.md entry (place it immediately under the * Your contribution here! line):

* [#226](https://github.com/capistrano/rails/pull/226): handle restore of multiple asset manifests on rollback - [@lennart](https://github.com/lennart)

Generated by :no_entry_sign: Danger

lennart commented 5 years ago

since this is neither a user-facing change nor can I see how to "test" this (apart from using this, and it seems to work).

is it possible to merge this pr?

lennart commented 4 years ago

@mattbrictson I resolved the issues you had in code and made a demo-run of deploy and rollback handling multiple-manifest files without breakage, see:

https://gist.github.com/lennart/d2eab716d5b589da869a7c6f86ec6cc2

lennart commented 4 years ago

um, when I take a look at this now, I realize there's a bug. targets and sources are mixed up... I'll fix this

lennart commented 4 years ago

ok, so now, starting at: https://gist.github.com/lennart/d2eab716d5b589da869a7c6f86ec6cc2#file-capistrano-deploy-and-rollback-log-L511

it lists the files in the assets_manifest_backup folder and then restores all backed-up manifests to public/assets/

lennart commented 4 years ago

@mattbrictson sounds good, I accepted your suggestion!