Fixes #12199 - rails 4 migration errors #14
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AccessibleAttributes pulls in all possible associations and models and sets them as attr_accessible for the model. Where needed, additional attr_accessible attributes have been added.
Some of our tests are checking if port 80 or 443 is included in the URL generated by lib/foreman/renderer.rb. However url_for has changed in Rails 4 and now it does not append the port even if explicitly added. If the protocol is specified, as it is in this case, then the url generated will just have the protocol (http or https) but not the port (80 or 443)
On Rails 4 such validations will fail when we try to create objects without explicitely assigning the id. Puppetclass.new(:config_group => config_group) would fail even for a valid config group, :config_group_id => config_group.id should be used instead. To avoid that, we validate the object, not the ID column
Changes that have to deal with how some of the internal Rails objects need a new syntax, like generating routes, exceptions, form builders. None of these are Rails 3 compatible.
…tion
On Rails 4 .scoped is deprecated. Calling .all on the model returns the
equivalent ActiveRecord relation object on Rails 4, but on Rails 3 it
returns an Array right away.
A proper replacement we can use is where(nil) - it's ugly but it returns
the same relation in both Rails 3 and 4. There are a couple of fixes too
on models such as bookmark.rb which return where conditions ({} and
similar) instead of a relation, which is also a deprecated behavior.
We could possibly substitute these by .all after the Rails 4 migration
if they feel too 'unidiomatic'.
This includes some changes that can be applied to Rails 3 without any issue and don't follow any particular pattern/deprecation, In general these are minor things we have to change. Other PRs with retrocompatible changes that aim to fix particular deprecations have been submitted separately. Sorry for the not-very-descriptive commit!
Rails 4 will default to PATCH which we have not implemented in our routes. This will cause at least one failure on taxonomies integration tests. It's the only one our test suite catches, however I'd expect other forms to fail on this when tested manually. We can keep this issue number as a reference and add commits with integration tests & fixes where needed.
…roller On Rails 4, .find will not default to .friendly.find so find_hostgroup, find_environment and find_host will fail for non numeric IDs. However, we can use from_param to find these objects. This is something we can do both on Rails 3 and Rails 4, whereas using .friendly isn't an option until friendly_id 5.0, which depends on Rails 4.
test/unit/compute_resource_test.rb contains a Fog.mock! without a Fog.unmock!. This causes test/functional/compute_resource_vms_controller_test.rb to fail only when ran through rake test on Rails 4. Fix is as simple as adding the missing unmock!.
Removes the rest of trends output from tests, since ad4db10 only removes the first part.
Problem: On Rails 3, using a lambda with no arguments works fine in an ActionMailer default. However on Rails 4, the lambda will whine because there is no handling of arguments and ActionMailer is trying to pass a HostMailer. Solution: Use Procs instead of lambda to allow it to work with and without arguments. How to reproduce the error: Checkout https://github.com/eLobato/foreman/tree/rails4_from_scratch, change the -> by ->(args) and put a debug breakpoint inside. Verify ActionMailer is passed. On Rails 3 this is not the case.
app/models/template.rb has a method locked? that relies on !Foreman.in_rake? to bypass the locked validation when running some operations through Rake such as migrations or cloning templates. On Rails 4 rake test is also interpreted as being 'in rake' and tests fail when running them through rake test but not when running template tests individually.
|
@elobato I don't see an easy solution that would work on both rails 3 and rails 4, unless you can think of anything? |
|
@johnpmitsch 👍 thanks , I'll clean up the branch in a bit and merge it. 😄 |
|
@johnpmitsch - meanwhile, cherry-picking this commit into your working VM allows you guys to work on the Katello rails4 branch with the rails4_from_scratch branch? |
|
@unorthodoxgeek yes, as long as we applied the fix for the actionpack gem I mentioned, we can cherry pick this commit and have a working rails4_from_scratch branch |
dLobatog
force-pushed
the
rails4_from_scratch
branch
from
95a89fe to
1577534
Compare
|
Merged as a47cd91, thanks @johnpmitsch ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are actually two parts to the issue of not being able to migrate with this branch. The first is https://gist.github.com/eLobato/0f5db50b5c93cc6c277c and can be temporarily fixed with rails/sass-rails#136 (comment) I am not sure of a long term fix for that atm.
Once that is patched, this error https://gist.github.com/johnpmitsch/96e5ba3629890931193a happens on a migration. This will fix that error by changing the Migrator class initializer arguments which have changed from rails 3 to rails 4