rails / sprockets-rails

Sprockets Rails integration
MIT License
579 stars 246 forks source link

Restore config.assets.manifest option. #107

Closed seantanly closed 10 years ago

seantanly commented 10 years ago

I'm putting this up in case the issue https://github.com/rails/sprockets-rails/pull/92 is lost.

@josh @guilleiguaran any status update relooking into the issue?

fountainheadpro commented 10 years ago

+1. On the need to support the config.assets.manifest option.

Configurable location for manifest is important to enable CDN based asset hosting. This feature was available in previous versions of the framework, so if the feature is deprecated, it still needs to be supported for couple of versions, so the users would be able to migrate.

Right not the backward compatibility is broken without ample warning.

johnnyshields commented 10 years ago

+1. This feature should be supported, not deprecated. It is required for CDN asset hosting, which is the best/cheapest way to host assets I've found.

guilleiguaran commented 10 years ago

Do we have a PR for this?

Are you sure that this should be implemented in sprockets-rails and not in sprockets directly? :grin:

johnnyshields commented 10 years ago

@guilleiguaran you removed this from Rails in this commit: https://github.com/guilleiguaran/rails/commit/66ed71fd6fbbdc25148459a91074974287318bf5

Please advise where it is better to implement, Sprockets or Sprockets-Rails, and I will make a PR.

arthurnn commented 10 years ago

:-1: I think we dont need to restore that config. To write to the right file you should use the Rake config, see and example https://gist.github.com/arthurnn/8813369 And to read, it will read any file that matches manifest*.json on config.assets.prefix folder.

That said, I think this already give us enough flexibility.

To use a CDN, you should just change the assets.prefix as this will, anyways, point to a different folder structure.

guilleiguaran commented 10 years ago

@arthurnn thanks for clarify this!!!

@johnnyshields wdyt about this?

johnnyshields commented 10 years ago

@arthurnn the goal of this option is to store the manifest.json in a location other than the config.assets.prefix folder, as we want to put manifest.json by itself under source control (without the compiled assets.)

For example, in my app I have it at /config/assets/<env>/manifest.json, while the assets are in /public/assets/*.

If I understand correctly, your solution cannot handle this structure?

rafaelfranca commented 10 years ago

Forgive me if you already explained this but why would you like to put the manifest file in the source control?

johnnyshields commented 10 years ago

Sure, the reason is that I am deploying my assets to a CDN (Amazon S3) save costs/bandwidth, while my Rails servers are cloud-based/virtual (Amazon EC2).

It's my company policy that all production deploys must be from a git tag. So the release process looks like this: 1) Compile assets, generate manifest 2) Push assets to S3 3) Check manifest into git, tag the git repo as "RELEASE-x.y.z" 4) Run the deploy script. The script prompts you for the git tag, then triggers each production server instance to remotely pull that tag from git, which includes the manifest file.

This has several advantages. For instance, when rolling back, I simply run the deploy script with the previous tag. Since the previous manifest is in source control and previous assets (w/ digest) are still on the CDN, there's no issue ensuring previous assets are used.

arthurnn commented 10 years ago

at Shopify, we have a similar process. What we do is, download the manifest.json to app/public/assets/manifest.json, and use the config.asset_host to have the CDN host.

manifest has no knowledge of CDN, host and stuff. It only knows the relative paths to translate the helpers. So I dont think you need to change the manifest class still.. Am I missing something?

johnnyshields commented 10 years ago

So, you push the manifest.json to the CDN, and the app reads it from the CDN?

In this case, how do you rollback to a previous version of your assets? Each time you push the manifest.json to the CDN, doesn't it overwrite the previous version?

arthurnn commented 10 years ago

@johnnyshields what we do is download the latest manifest.json local in each rails server. and on cdn we have the manifest uploaded with a assets version. You dont want your rails server hitting another server to get information from the manifest.json.

johnnyshields commented 10 years ago

OK, so manifest.json lives on each Rails server. How you do download it--from the CDN? When you rollback, do you download the older manifest.json version?

I prefer a policy that "everything that goes to Rails server comes directly from Git". Since the manifest.json needs to be versioned (to facilitate rollbacks, etc.) it is reasonable for it to be in Git.

fountainheadpro commented 10 years ago

I'm using this the same way. Assets are compiled and pushed to CDN, while manifest.json is located on the server to help locate the assets. When we were on Rails 3, we my manifest was located in the config directory and public/assets directory was ignored by git and was only used to push assets to CDN.

seantanly commented 10 years ago

Same here. An additional benefit is that after precompiling and syncing our assets to CDN, we can easily run the app in production mode to test against the uploaded compiled assets on CDN before committing the manifest file to scm for deployment.

johnnyshields commented 10 years ago

To summarize:

1) when serving assets via CDN, the manifest.json-in-Git approach offers several advantages: pre-deploy testing, rollback, certainty of which assets were deployed, etc.

2) it is possible to use CDN assets without storing manifest.json in Git (as @arthurnn does), but the above advantages will be lost.

3) in order to cleanly put manifest.json in Git, it should be generated in a directory other than the compiled assets dir (which is .gitignore'd.) For this, we require to restore the config.assets.manifest option.

Does everyone agree with this? If so, can we move forward with restoring this option?

rabbitt commented 10 years ago

:+1:

johnnyshields commented 10 years ago

@guilleiguaran @arthurnn may we move forward with this?

schneems commented 10 years ago

Configurable location for manifest is important to enable CDN based asset hosting.

Why? I've used a number of CDN's and i've never been required to provide them with my manifest (example: https://devcenter.heroku.com/articles/using-amazon-cloudfront-cdn). This is a very long thread, sorry if it's already been answered.

johnnyshields commented 10 years ago

@schneems please read above. The manifest is not provided to the CDN; it's pushed to the app servers. As discussed above there are both manifest and non-manifest approaches; using manifest is optional but provides a number of advantages in terms of deploy certainty and rollback.

matthewd commented 10 years ago

@johnnyshields could you elaborate on why manifest.json can't live in the compiled assets directory? Once you git add it once, the fact that its directory is in .gitignore becomes irrelevant.

johnnyshields commented 10 years ago

Yes:

  1. Working on a project with many co-workers the compiled assets directory may get deleted inadvertently when users clean out temp files.
  2. Many third-party git front-ends do not let you add/update files that are in .gitignore'd dirs (Rubymine and TortoiseGit do not)
  3. Correct if wrong but I believe that updates to such .gitignore'd files are ignored when committing, even if the file was added previously.
  4. By any measure, it's not a best practice to do so. It just makes more sense for it to live in the config dir.
johnnyshields commented 10 years ago

Another advantage not mentioned above--in Rails 3 you could set different paths for staging / production manifests (i.e. env can be interpolated in the path). This allows you to test your staging server with the new asset set while your production stays on the old one, until you are ready to upgrade.

@guilleiguaran may I raise a PR to restore the config.assets.manifest option?

guilleiguaran commented 10 years ago

Please do.

On Fri, Mar 14, 2014 at 12:02 AM, Johnny Shields notifications@github.comwrote:

Another advantage not mentioned above--in Rails 3 you could set different paths for staging / production manifests (i.e. env can be interpolated in the path). This allows you to test your staging server with the new asset set while your production stays on the old one, until you are ready to upgrade.

@guilleiguaran https://github.com/guilleiguaran may I raise a PR to restore the config.assets.manifest option?

Reply to this email directly or view it on GitHubhttps://github.com/rails/sprockets-rails/issues/107#issuecomment-37617034 .

schneems commented 10 years ago

I've been looking into this, and I'm pretty sure we can achieve what you want without this feature.

Your CDN argument seems to rely on checking in the manifest file. You can manually whitelist just the manifest file using a ! in your .gitignore file like this:

public/assets/*
!public/assets/manifest-*.json

Now only the manifest is added to git. You don't need a feature in Rails to achieve this functionality.

you could set different paths for staging / production manifests

This sounds like an anti-feature. If someone was brought on to work on your app (consultant, new hire, whatever) they wouldn't expect the file to be in different locations for different environments. I also think you shouldn't be using a staging environment but instead use a production environment with staging config, see here for detailed explanation: https://devcenter.heroku.com/articles/deploying-to-a-custom-rails-environment?preview=1

If you really want to change the location of your manifest file you can still do this manually by using rake enhance http://rake.rubyforge.org/classes/Rake/Task.html#M000112 in your Rakefile http://www.dan-manges.com/blog/modifying-rake-tasks and an initializer that moves the manifest to the any location you want at boot time.

In short, i don't think that moving the manifest is a Rails happy path feature that needs to be supported by Rails core. You can get all you need (and more!) through existing functionality.

johnnyshields commented 10 years ago

@schneems firstly thank you for the lengthy response. While we have different ideas here, we share the common goal of building an awesome framework. I've been on the road (at EmberConf now!) so apologize for the delay in replying.

You can manually whitelist just the manifest file using a ! in your .gitignore file

1) As the actual assets I serve live on a CDN, I consider public/assets to be a like "temp" directory, while my manifest.json is a "non-temp" file. Mixing temp and non-temp files in a folder is possible but not ideal.

2) I use Capistrano for deploys. When deploying via remote copy (local to staging server), although I only want to push manifest.json, I must push everything in the assets directory which takes a long, long time. Capistrano supports only directory blacklist (see :copy_exclude), not blacklist + whitelist like .gitignore.

different paths for staging/production manifests... is an anti-feature

It is a requirement for me that staging and production environments be completely isolated. While it is possible to set separate staging/prod S3 buckets per environment as mentioned in Heroku article, it is not possible (currently) for a single manifest file to describe two sets of assets for two different buckets (as far as I'm aware).

you shouldn't be using a staging environment but instead use a production environment with staging config... see Heroku article

Regardless of having staging/production as one or two environment configs, I still require separate staging and production S3 buckets. As per above, I need a separate manifest for each bucket.

If you really want to change the location of your manifest file you can still do this manually by using rake enhance

This use case is common enough (@seantan @fountainheadpro @rabbitt) to justify a one-line lib change + one test case to support the configuration of this feature, rather than monkey-patches and/or enhancing Rake tasks (which is brittle.) Note also this was supported in Rails 3, and it was removed without giving an upgrade path for those using it.

johnnyshields commented 10 years ago

Awesome, thanks so much for merging this.

ryanjohns commented 10 years ago

This option doesn't seem to have any effect for me. I'm upgrading to rails 4.1.0 and ruby 2.1.1 with sprockets-rails 2.1.3. I have config.assets.manifest = "#{Rails.root}/config" in production.rb like used to work with rails 3.2.X. When I precompile my assets with RAILS_ENV=production bundle exec rake assets:precompile it still generates the manifest-.json file in public/assets. Any idea what I'm doing wrong? Thanks

johnnyshields commented 10 years ago

@ryanjohns you're correct there's an issue with the Rake task--this is my error. I will have a fixed branch shortly.

johnnyshields commented 10 years ago

@ryanjohns can you please try:

https://github.com/johnnyshields/sprockets-rails https://github.com/johnnyshields/sprockets

I have raised PRs here:

https://github.com/rails/sprockets-rails/pull/136 https://github.com/sstephenson/sprockets/pull/560

Apologies again for the issue.

ryanjohns commented 10 years ago

@johnnyshields No worries. Thanks for looking into it so quickly!

I'm running into other issues testing this because my project depends on sass-rails which is now locked to sprockets <= 2.11.0 due to https://github.com/rails/sass-rails/issues/191. There doesn't seem to be any consensus on how to fix sass-rails at the moment so I'm not really sure how to proceed.

Thanks again.

ryanjohns commented 10 years ago

@johnnyshields I removed my dependence on sass-rails and pointed sprockets/sprockets-rails to your forks and everything works exactly as expected. Thanks again for your help.

johnnyshields commented 10 years ago

@ryanjohns my pleasure, please also keep an eye out for correct function in prod environment.

johnnyshields commented 10 years ago

@ryanjohns, after discussion with Sprockets gem (thanks @josh) https://github.com/sstephenson/sprockets/pull/560 it was decided that the manifest option needs to be a full path + filename, not just a dir like Rails 3.2.

So, please set something like:

config.assets.manifest = "#{Rails.root}/config/manifest.json"

Please also use github master of sprockets gem, not my fork. (Continue to use my fork of sprockets-rails until PR https://github.com/rails/sprockets-rails/pull/136 is merged, however)

ryanjohns commented 10 years ago

@johnnyshields Yep, got it. I was following your conversation on that PR. :) Really appreciate your continued diligence on this. Also, agree with you guys that it should be a full path + filename. I was using manifest.json (without the hash) and just relying on the fact that it chooses the first matching file so this feels more robust. Thanks so much.