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

fixes #13244 - update Rails to 4.2.6 #3294

Closed
wants to merge 1 commit into from

Conversation

domcleal
Copy link
Contributor

@domcleal domcleal commented Mar 8, 2016

  • Add responders gem to support class-level respond_to usage
  • Replace foreigner with native Rails 4.2 FK support
    • uses a patch from Rails 5 to support Foreigner's
      foreign_key_exists? helper for full compatibility
    • foreign_keys should be avoided in favour of the higher level
      methods as it throws a NotImplementedError on sqlite3
  • Update DB adapter versions to match ActiveRecord
  • Enable exceptions from after_commit handlers to detect more errors
  • Change deprecated application config settings
  • Remove test:lib chaining on rake test task
    • 4.2 runs lib tasks automatically now, as test:run is redefined
      to all _test files within test/ rather than units+functionals.
      The task is still needed for the jenkins:* tasks.
  • Fix deprecation of mailer #deliver method, change to #deliver_now
  • Change CSRF test to use generated, not static tokens
    • 4.2 changes CSRF tokens to be different on every request and
      validated against the session, so use its generator to test the
      controller behaviour instead of hardcoding tokens.

Please note this is against the rails42 branch, not develop, per https://groups.google.com/forum/#!topic/foreman-dev/6ANhWVGDzNA. Katello tests will fail, you'll need to ignore them on this branch until it's fixed.

- Add responders gem to support class-level respond_to usage
    - http://edgeguides.rubyonrails.org/4_2_release_notes.html#respond-with-class-level-respond-to
- Replace foreigner with native Rails 4.2 FK support
    - uses a patch from Rails 5 to support Foreigner's
      `foreign_key_exists?` helper for full compatibility
    - `foreign_keys` should be avoided in favour of the higher level
      methods as it throws a NotImplementedError on sqlite3
- Update DB adapter versions to match ActiveRecord
- Enable exceptions from after_commit handlers to detect more errors
- Change deprecated application config settings
- Remove test:lib chaining on rake test task
    - 4.2 runs lib tasks automatically now, as test:run is redefined
      to all _test files within test/ rather than units+functionals.
      The task is still needed for the jenkins:* tasks.
- Fix deprecation of mailer #deliver method, change to #deliver_now
- Change CSRF test to use generated, not static tokens
    - 4.2 changes CSRF tokens to be different on every request and
      validated against the session, so use its generator to test the
      controller behaviour instead of hardcoding tokens.
@@ -57,4 +57,7 @@

# Enable automatic creation/migration of the test DB when running tests
config.active_record.maintain_test_schema = true

# Maintain standard order of running tests in case of leaking changes
config.active_support.test_order = :sorted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This can make our tests order dependant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran a few tests with it set to random and I'm sure they already are order dependent (since :sorted is the current Rails/develop behaviour), and we're not alone.

I think the affected test cases are:

  • Api::V2::OperatingsystemsControllerTest
  • Api::V2::OsDefaultTemplatesControllerTest
  • Api::V2::OverrideValuesControllerTest
  • Api::V2::ParametersControllerTest
  • Api::V2::PuppetclassesControllerTest
  • Api::V2::ReportsControllerTest
  • Api::V2::RolesControllerTest
  • Api::V2::SmartClassParametersControllerTest
  • Api::V2::SmartProxiesControllerTest
  • Api::V2::SmartVariablesControllerTest
  • Api::V2::SubnetsControllerTest
  • Api::V2::TasksControllerTest
  • Api::V2::TemplateCombinationsControllerTest
  • SeedsTest

Jenkins runs:

I'll take a closer look at each test soon and see if I can address the root causes in develop, then when it's stable we can flip this to random on this branch. Is that OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started tracking fixes under http://projects.theforeman.org/issues/14155.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense, I thought they were running in random order using the --seed param until now

@dLobatog
Copy link
Member

@domcleal Left a couple of comments I didn't understand - other than that 👍 , thanks for doing this

@lzap
Copy link
Member

lzap commented Mar 11, 2016

LGTM, Foreman Discovery 5.0.2/develop works fine. All tests are green.

@dLobatog
Copy link
Member

Merged as ccda759, thanks @domcleal

@dLobatog dLobatog closed this Mar 11, 2016
@domcleal
Copy link
Contributor Author

domcleal commented Apr 20, 2016

The rails42 branch/commits have now been merged to develop as 85a9714.

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