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

Added option ':assets_manifests' to support custom manifest file path #216

Merged
merged 4 commits into from
May 31, 2018

Conversation

tkuchiki
Copy link
Contributor

Added option ':assets_manifest' to support custom manifest file path.

Without this option and set config.assets.manifest in config/application.rb, the following error message appears.

DEBUG [30163db1] Running /usr/bin/env ls /path/to/releases/xxx/public/assets/.sprockets-manifest* as yourname@yourhost
DEBUG [30163db1] Command: cd /path/to/releases/xxx && /usr/bin/env ls /path/to/releases/xxx/public/assets/.sprockets-manifest*
DEBUG [30163db1] 	ls: 
DEBUG [30163db1] 	cannot access /path/to/releases/xxx/public/assets/.sprockets-manifest*
DEBUG [30163db1] 	: No such file or directory
DEBUG [30163db1] 	

DEBUG [30163db1] Finished in 0.325 seconds with exit status 2 (failed).
DEBUG [3e09dcf0] Running /usr/bin/env ls /path/to/releases/xxx/public/assets/manifest*.* as yourname@yourhost
DEBUG [3e09dcf0] Command: cd /path/to/releases/xxx && /usr/bin/env ls /path/to/releases/xxx/public/assets/manifest*.*
DEBUG [3e09dcf0] 	ls: 
DEBUG [3e09dcf0] 	cannot access /path/to/releases/xxx/public/assets/manifest*.*
DEBUG [3e09dcf0] 	: No such file or directory
DEBUG [3e09dcf0] 	

DEBUG [3e09dcf0] Finished in 0.293 seconds with exit status 2 (failed).
WARN Rails assets manifest file not found.
(Backtrace restricted to imported tasks)
cap aborted!

config.assets.manifest is used in https://github.com/rails/sprockets-rails.

@capistrano-bot
Copy link

capistrano-bot commented Nov 20, 2017

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.

Generated by 🚫 Danger

@mattbrictson
Copy link
Member

Hi, this seems like a useful feature. Thanks for the PR! I appreciate the tweaks you made for coding style and adding a good CHANGELOG entry.

Before merging this, I would like to suggest some minor changes to make the code a bit more clear.

  1. How about using Array() to coerce the user-supplied :assets_manifest value to an array if it is not one already. This allows the user to supply a single value or an array of values. It also means that the default behavior can use the same mechanism, and not be a special case.
  2. Set the default value of :assets_manifest in load:defaults instead of relying on a special nil conditional for the default behavior. This should simplify the logic.

Perhaps the default value could be something like this:

# Default set in load:defaults
set :assets_manifest, -> {
  %w[.sprockets-manifest* manifest*.*].map do |pattern|
    release_path.join("public", fetch(:assets_prefix), pattern)
  end
}

Also, could you rebase and fix the conflict? Thanks!

@tkuchiki
Copy link
Contributor Author

@mattbrictson Thank you for your feedback! I think you’re right.
I will fix it.

@tkuchiki tkuchiki changed the title Added option ':assets_manifest' to support custom manifest file path Added option ':assets_manifests' to support custom manifest file path Nov 24, 2017
@tkuchiki
Copy link
Contributor Author

@mattbrictson Fixed.

How about using Array() to coerce the user-supplied :assets_manifest value to an array if it is not one already. This allows the user to supply a single value or an array of values. It also means that the default behavior can use the same mechanism, and not be a special case.

assets_manifest rename to assets_manifests to clarify that it is a Array.
Would this be okay?

@webchi
Copy link

webchi commented Mar 30, 2018

Very need this fix

@webchi
Copy link

webchi commented Mar 30, 2018

Actually there is manifest file in /public/pack directory, but capistrano/rails watch only /public/assets Need to deal with it))
[10 minutes later...]
Fixed by set :assets_prefix, 'packs'

@mattbrictson
Copy link
Member

This looks good! I'll test it this week with an actual deploy and merge it if it all works.

@mattbrictson
Copy link
Member

All checks out 👍

Congrats on your first contribution to the project!

@mattbrictson mattbrictson merged commit 13cea18 into capistrano:master May 31, 2018
@mattbrictson
Copy link
Member

I plan to push a new version of the gem this weekend.

@tkuchiki
Copy link
Contributor Author

Thank you for reviewing this PR.
I'm looking forward to release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants