Skip to content
This repository has been archived by the owner. It is now read-only.

Fix asset precompile on Cloud Foundry #636

Closed
yozlet opened this issue May 5, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@yozlet
Copy link
Contributor

commented May 5, 2015

Asset precompile is failing because it tries to initialize the Rails app, which tries to access the DB at initialization time, which fails because the buildpack doesn't pass in the DB connection info at compile time.

@dlapiduz identified the specifics of the Rails initialization problem: User.rb requires our custom two-factor extension which extends ActiveRecord::Base at init time. Removing the two-factor references from User.rb lets precompilation happen without error, but that's not really an option. (The errors are actually coming from attr_encrypted, and removing that may be an option since we could move to encrypted RDS instead, and that would honestly be much better than attr_encrypted anyway. But it may be too much work, and I'm not certain that removing attr_encrypted would actually solve the problem.)

Possible solutions:

  1. Do asset precompilation before deployment (in the CI system) and remove it from the buildpack entirely.
  2. Tweak the buildpack to enable DB connection during the compile phase.
  3. Tweak the buildpack to move the asset precompilation from the compile phase to the run phase.
  4. Find a way to remove the DB connection dependency from the app initialization.
@yozlet

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2015

Note on solution 2: Heroku used to have a Labs feature called user env compile to make user environment variables available to the compile phase, but it doesn't seem to be in the ruby buildpack any more (or if it is, it isn't obvious how to enable it).

yozlet added a commit that referenced this issue May 5, 2015

Shameful massage by bad-tempered man
Nasty fix for DB encryption key supply
Horrid fix for #636
@yozlet

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2015

Fixed in an easy-but-awful way by simply catching the NoMethodError exception. (Thanks, @dlapiduz)

@yozlet yozlet closed this May 6, 2015

@NoahKunin

This comment has been minimized.

Copy link
Contributor

commented May 6, 2015

@yozlet can you speak a little more about the awfulness?

@yozlet yozlet referenced this issue May 6, 2015

Closed

Migrate to Cloud Foundry for ATO #639

7 of 7 tasks complete

@jackiekazil jackiekazil added this to the Sprint 29 - 05/11/2015 milestone May 11, 2015

@yozlet yozlet referenced this issue May 26, 2015

Open

Remaining Cloud Foundry migration tasks #662

4 of 5 tasks complete
@yozlet

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2015

@NoahKunin: Oops, sorry for the slow response!

Here is the awful code in question:

rescue NoMethodError
# Yes, this is an awful thing. Glad you noticed.
# Short version: This mixin causes Rails to attempt to reach the DB
# at initialization time, which is a bad thing if you're in the
# compile stage rather than run stage (when trying to build a
# droplet for Cloud Foundry) and you just want to precompile assets.
# Longer discussion: https://github.com/18F/myusa/issues/636
end

See the comments for cause and fix. After the 4 options listed in the issue description, I went for option 5 (paper over the problem while loudly whistling). In practice this solution is probably the best one for the moment; it's cheating, but unlikely to backfire on us.

The ideal one would be option 4: untangle attr_encrypted and make it not depend on the DB for asset precompilation, but that requires more Rails surgery than any of us can stomach right now.

Does that explain things enough?

@monfresh

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2015

Are you sure your asset precompilation is working? I cloned this repo just now and when I try to precompile the assets locally (after setting the proper env vars), it's failing due to a known issue with using validates_acceptance_of with virtual attributes. If I comment out all validates_acceptance_ofvalidations, precompilation works fine, even when I undo the rescue NoMethodError you added.

The solution is to add all of the attributes (even non-virtual ones) to attr_accessor and make sure that line is placed above the validations:

# in app/models/user.rb
attr_accessor :just_created, :auto_approve, :terms_of_service, :two_factor_required
validates_acceptance_of :terms_of_service
...
# in lib/doorkeeper_patches/models/application.rb
attr_accessor :terms_of_service_accepted
validates_acceptance_of :terms_of_service_accepted,
...

To test asset precompilation locally, I didn't run rake db:setup on purpose so that I wouldn't have a DB and so any code that attempted to connect to the DB would result in an exception. I then ran rake assets:precompile RAILS_ENV=production.

For a more generic test that works whether or not you already have the DB created, you can test by specifying a nonexistent DB, like this:

env -i GEM_PATH=$GEM_PATH \
       PATH=$PATH \
       RAILS_ENV=production\
       DATABASE_URL=mysql2://user:pass@127.0.0.1/does_not_exist_dbname \
       /bin/sh -c 'bundle exec rake --trace  assets:precompile'
@yozlet

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2015

Wow, thanks @monfresh, I hadn't realised that was the problem. I'll give that a go.

@yozlet

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2015

Initial investigation suggests that the problem is a slightly different (but related) one - the fields in the validations are real, not virtual.

The compilation problem is fixed by adding attr_accessor declarations, but since the fields in question are real DB fields, it has the unfortunate side-effect of clobbering some of the ways of setting those values. This may only affect tests rather than real usage but I'm not sure, and I'd rather not risk it until we have a better understanding of what's happening here.

@monfresh

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2015

Agreed. It was premature of me to suggest using attr_accessor for real fields as well. According to the comments in the Rails issue I linked to, this Rails bug only happens with virtual fields, so there must be something else that is affecting real fields.

However, taking a step back, I would question whether validates_acceptance_of is really needed and/or whether a database column is really needed. That validation is meant to be used to make sure that users check a particular checkbox on a form, such as accepting terms of service. If all users must accept the terms of service, then there is no need to create a database column for that attribute. Rails takes care of it for you as a virtual attribute.

I also noticed that you have 2 similar validations related to terms of service, one in the User model that uses the virtual terms_of_service attribute, and one in the Doorkeeper Application model that uses the database column terms_of_service_accepted. Is there a reason for having both of these?

You also have another acceptance validation for the database column two_factor_required. How is this used from a user perspective? Does the user get to decide whether or not their two factor is required? Could you describe to me what is being validated here? Perhaps I can point you to a better validation method.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.