Fix for parsing ls output in detect_manifest_path

[januszm]

NOTICE: we probably should use a different approach than parsing ls results

[januszm]

O wow, there are several forks that include almost similar fix. Why the fix is not yet merged?

[januszm]

bump, I've seen some new commits that were merged to master in the past few days. Could you guys also take a look at this PR ?

[maartenvanvliet]

👍

[mcfiredrill]

Having the same issue.

[januszm]

@kirs what do you think?

[carld]

Same here

diff --git a/lib/capistrano/tasks/assets.rake b/lib/capistrano/tasks/assets.rake
index 803cefa..465c5cc 100644
--- a/lib/capistrano/tasks/assets.rake
+++ b/lib/capistrano/tasks/assets.rake
@@ -66,9 +66,11 @@ namespace :deploy do
         within release_path do
           backup_path = release_path.join('assets_manifest_backup')

+          manifest_path = detect_manifest_path.gsub(/\n/, ' ')
+
           execute :mkdir, '-p', backup_path
           execute :cp,
-            detect_manifest_path,
+            manifest_path,
             backup_path
         end
       end
--

[hishammalik]

+1 for the fix

[aligo]

+1 for this

[czimmerman]

Any reason this hasn't been merged yet? Thank you for this.

[joren]

👍 fixed it for me as well.

[jemiam]

👍

[LuanJames]

+1

[mcfiredrill]

Have the maintainers seen this PR? Is this a good fix? Any feedback?
This is not an issue if you only have one manifest file.
I suspect there might be an underlying issue, or is there a good use case for having more than one manifest file?

[mattbrictson]

I don't have a test case to prove it, but I think this approach will fail during restore_manifest. I think a more robust solution is needed that loops over each manifest file rather than concatenating multiple paths with spaces, which is prone to error when you pass that string around to different shell commands.

TBH, the lack of unit tests (or any kind of tests whatsoever) in this project makes me hesitant to add code to fix an edge case like this. I'd worry about inadvertently breaking something.

However, in place of tests, if you can post Capistrano logs that show a PR successfully doing both a backup and a restore with multiple manifests, I'll consider merging it.

Sorry for being a downer, I know this is a big sore spot for those running into the multiple manifests problem. I just want to make sure we do this right.

Also, does anyone have a theory for how multiple manifests come to exist in the same assets directory in the first place?

[mcfiredrill]

@mattbrictson Thanks alot for taking the time to respond.

I had a feeling there was a more robust solution...

Maybe its worth getting this merged to help in the short term though.
When does restore_manifest happen exactly? I could try to test it.

[mattbrictson]

When does restore_manifest happen exactly?

During cap deploy:rollback (see rollback docs).

I could try to test it.

That would awesome!

[januszm]

Guys, a fix that works, even if as simple as mine, is better than no fix.

I see two main issues here:

1. For some reason sometimes multiple manifest files appear. This is a different story, an issue that probably should be addressed somewhere else and deserves its own pull request.
2. A code that relies on parsing output from LS command was added, whoah! long time ago. This was not the best solution for the problem the original developer tried to solve. Parsing output from ls is unsafe. If there's more than one result from ls, most probably the output string will contain the newline characters.

In my humble opinion you should merge this fix and work on resolving the issue with multiple manifest files in a separate thread. I've been using this fix for months already, I've made dozens of deploys, even one rollback, and never had issues with it.

[kirs]

Thank you all for the discussion and sorry for the late reaction.

[kirs]

I agree that there should be a better way than parsing ls output, but so far it's the only approach that we use in Capistrano (https://github.com/capistrano/capistrano/blob/master/lib/capistrano/tasks/deploy.rake#L193) - but we're open so suggestions!

[kirs]

v1.1.6 released 🚀

[mcfiredrill]

Great, I can at least stop using a fork for now.
If there's an issue with rollbacks I imagine we'll hear about it soon... 💦