jnraine / munkiserver

Visit https://github.com/munkiserver/munkiserver for active development — this repository is no longer maintained
85 stars 27 forks source link

Rails 3.1/3.2 support #134

Closed treydock closed 12 years ago

treydock commented 12 years ago

This was tested with Rails 3.2.2.

A new step is required before being able to start the application in production mode

bundle exec rake assets:precompile RAILS_ENV=production

That will precompile all assets to public/assets

rickychilcott commented 12 years ago

Jordan, since Trey and I worked on this, do you mind pulling this in and checking it over? It's a big change and I want to make sure we have multiple eyes on it.

treydock commented 12 years ago

I just pushed this to my production system...it's relatively small only about 20 systems and maybe 80 packages, but I already notice a huge increase in load times for the web interface.

rickychilcott commented 12 years ago

So it's less performant?

On Mar 12, 2012, at 11:14 AM, treydock wrote:

I just pushed this to my production system...it's relatively small only about 20 systems and maybe 80 packages, but I already notice a huge increase in load times for the web interface.


Reply to this email directly or view it on GitHub: https://github.com/jnraine/munkiserver/pull/134#issuecomment-4453882

treydock commented 12 years ago

LOL that's a really bad typo. No...a huge decrease in load times, sorry. It's much more responsive especially loading the packages view with all the images. The latest commit helps as it adds the paperclip images to asset pipeline.

treydock commented 12 years ago

Ricky,

Did you get this message when running bundle install?


Post-install message from sqlite3-ruby:

#######################################################

Hello! The sqlite3-ruby gem has changed it's name to just sqlite3.  Rather than
installing `sqlite3-ruby`, you should install `sqlite3`.  Please update your
dependencies accordingly.

Thanks from the Ruby sqlite3 team!

<3 <3 <3 <3

#######################################################

If there's no specific reason to keep sqlite3-ruby, this may be worth adding...let me know


diff --git a/Gemfile b/Gemfile
index 9ed574c..56e81db 100644
--- a/Gemfile
+++ b/Gemfile
@@ -2,7 +2,7 @@
 source 'http://gemcutter.org'

 gem "rails", "3.2.2"
-gem "sqlite3-ruby", :require => "sqlite3"
+gem "sqlite3"
 gem "execjs"
 gem "therubyracer"
 gem 'will_paginate', '~> 3.0' # version added for rails 3 compatibility
rickychilcott commented 12 years ago

I did not. You should only have this line:

gem "sqlite3-ruby", :require => "sqlite3"

If you need a specific version, change it to:

gem "sqlite3-ruby", "1.3.4", :require => "sqlite3"
jnraine commented 12 years ago

Just looked over the commits and things look good. One thing jumped out at me though. The default icons were moved to lib but paperclip version of those assets were also saved within "app/assets/paperclip/...". If I remember correctly, those assets will never be used as paperclip will regenerate the different sizes upon running the bootstrap rake tasks. I imagine we can remove those images from the repo altogether and save on some extra bloat.

Thoughts?

treydock commented 12 years ago

I didn't even notice the ones in lib/. Three options to make use of the asset pipeline with these

The only reason I'd suggest vendor/assets is they are third-party assets which is really just organizational.

I think removing app/assets/paperclip from the repo is fine. This will be especially useful if anyone uses Capistrano. I just deployed rails_31 branch to my production server today using Capistrano and had to go back and remove the paperclip directory from git.

treydock commented 12 years ago

Forgot to add...I have no idea if this is an issue, but since the current config in production does not compile assets on the fly, it may be good to test adding a new icon, and making sure it works without running the rake task.

rickychilcott commented 12 years ago

That was an area I was not sure about. Now that I've looked into it, I I don't think the asset pipeline should be involved. I think we can remove all the files in app/assets/paperclip/ and keep the app/models/icon.rb code the same. Even though the asset pipeline is a good improvement to Rails, it's not intended to replace the public/ folder entirely.

Also, because of this, we don't need commit 3da9f7fe92777800dd391b595ba02e7f7e68c408.

Does that sound right?

treydock commented 12 years ago

Ya that sounds right, that commit can be reverted. Since paperclip will serve up the images it won't be necessary to use asset pipeline.

treydock commented 12 years ago

Ricky, since we won't put the paperclip uploads in asset pipeline we may still want to change the path in the icon model. It may conflict if we use '/assets'. I think without asset pipeline or a route, we'd need to move the path to somewhere in public. The default for Paperclip I think is public/system.

When running some tests I found that not using the asset pipeline was better because otherwise new uploaded icons wouldn't be visible (at least not without running rake precompile task). Once the asset pipeline was disabled though the images couldn't be accessed until I changed the icon paths and put the folder in public.

This is what I changed to make it work...the paths may want to be altered to stay in line with how they appear now


diff --git a/app/models/icon.rb b/app/models/icon.rb
index a69708b..bd3457e 100644
--- a/app/models/icon.rb
+++ b/app/models/icon.rb
@@ -11,8 +11,8 @@ class Icon < ActiveRecord::Base
   has_attached_file :photo,
                     :styles => { :tiny => ["32x32>", :png], :small => ["64x64>", :png], :medium => ["128x128>", :png], :large => ["256x256>", :png] },
                     # :styles => { :tiny => "32x32>", :small => "64x64>", :medium => "128x128>", :large => "256x256>" },
-                    :path => ":rails_root/app/assets/paperclip/:attachment/:id/:style/:filename",
-                    :url => "/assets/:attachment/:id/:style/:filename"
+                    :path => ":rails_root/public/:attachment/:id/:style/:filename",
+                    :url => "/:attachment/:id/:style/:filename"

   # A shortcut to get the url for self.photo
   def url(type = nil)
rickychilcott commented 12 years ago

@treydock That looks right to me. we might want it to be

has_attached_file :photo,
                     :styles => { :tiny => ["32x32>", :png], :small => ["64x64>", :png], :medium => ["128x128>", :png], :large => ["256x256>", :png] },
                    :path => ":rails_root/public/paperclip/:attachment/:id/:style/:filename",
                    :url => "/:attachment/:id/:style/:filename"

just so we keep paperclipped assets namespaced. No matter what change we do, users will like have to re-import the icons. Agreed?

treydock commented 12 years ago

Shouldn't the path in public match the URL? I tried what you suggested but all images break. I make this change it works.


+                    :url => "/paperclip/:attachment/:id/:style/:filename"
jnraine commented 12 years ago

I was thinking the same Trey. How about we use a less implementation specific path? Perhaps :rails_root/public/icons/:attachment/:id/:style/:filename and icons/:attachment/:id/:style/:filename?

rickychilcott commented 12 years ago

I dig that.

Sent from my iPod

On Mar 14, 2012, at 6:43 PM, Jordan Rainereply@reply.github.com wrote:

I was thinking the same Trey. How about we use a less implementation specific path? Perhaps :rails_root/public/icons/:attachment/:id/:style/:filename and icons/:attachment/:id/:style/:filename?


Reply to this email directly or view it on GitHub: https://github.com/jnraine/munkiserver/pull/134#issuecomment-4510005