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

Upgrade to rails 5.1 #7514

Merged
merged 52 commits into from Aug 12, 2017
Merged

Upgrade to rails 5.1 #7514

merged 52 commits into from Aug 12, 2017

Conversation

SuperTux88
Copy link
Member

@SuperTux88 SuperTux88 commented Aug 7, 2017

This PR still depends on 2 unreleased gems for rails 5.1 compatibility.

ToDos after merging:

  • File a follow-up to merge old migrations we no longer need.
  • Adjust installation docs to use db:migrate instead of db:schema:load

@cmrd-senya
Copy link
Member

Huge job! 👍

@SuperTux88
Copy link
Member Author

And my workaround in config/initializers/assets.rb doesn't work on travis (works locally in development mode).

I pushed another workaround-commit. When importing _bootstrap with underscore, it works ... but pronto doesn't like the underscore ...

If anybody has an idea on how to solve this (in a cleaner way), I'm open for help ...

@SuperTux88
Copy link
Member Author

And the updated schema.rb doesn't work with postgres ... :/ Maybe we should switch to postgres and maybe the postgres-version of the schema.rb works with mysql?

Anyway ... I go to bed and try that at some other time.

@denschub denschub added this to the 0.7.0.0 milestone Aug 7, 2017
@SuperTux88
Copy link
Member Author

Oh, and mobile cukes broke with timeouts ... that worked locally. Need to check what's the problem there. Maybe some more caches that are there locally but not on travis?

@denschub
Copy link
Member

denschub commented Aug 7, 2017

All migrations are dead, since they need explicit Rails version information now:

Directly inheriting from ActiveRecord::Migration is not supported. Please specify the Rails release the migration was written for:

  class CreateSchema < ActiveRecord::Migration[4.2]

@SuperTux88
Copy link
Member Author

The problem with cucumber is weird, for example here ... everything worked, until one cuke failed and every following cuke failed with timeout!?

@denschub
Copy link
Member

denschub commented Aug 7, 2017

Pushed some commits. I fixed the migrations to be working with Rails 5.1, making sure that you can run all migrations from an empty database.

As for the schema.rb, this is tricky. Basically, according to rails/rails#26209, it is not a bug that the schema.rb is not compatible across databases, but expected behavior. While it's usually good to have it checked in, it makes no sense if you share the schema between databases.

So I decided to remove it. For initial setup, users have to run migrations, but I'll file a follow-up for merging old migrations so we can reduce the number a bit. Also, as a side effect, travis now tests our migrations across databases, so that's cool. We have to change our documentation for installing, but that's doable.

I'll fix any issues with the migrations coming up here (for example, MySQL just failed for some reason)

@SuperTux88
Copy link
Member Author

I reverted d511f3c and replaced it with 49df3e6 ... that fixed the ID bigint/auto-increment problem.

@SuperTux88 SuperTux88 force-pushed the rails5 branch 5 times, most recently from 082dd33 to 5383126 Compare August 11, 2017 02:18
Copy link
Member

@denschub denschub left a comment

Choose a reason for hiding this comment

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

Approval for all of @SuperTux88's commits, he will review mine!

@SuperTux88
Copy link
Member Author

I reviewed @denschub commits (thanks for the help ❤️) and approve the (can't approve my own PR)

I'll rebase this once again and use mobile_fu release for now. If they release a new version of mobile-fu before we release 0.7.0.0, we can switch that back.

@denschub
Copy link
Member

👍

SuperTux88 and others added 24 commits August 12, 2017 15:39
* don't use `double` for queries
* use `second` instead of `at()`.
* load records to array to check them
* test that only 15 people are returned
The change in assets.rb should be enough, but it doesn't work, because
sprockets `after_initialize` runs before it and initializes sprockets
with unfiltered paths.

But the trick with the underscore works, because bootstrap-sass has
named the file `_bootstrap.scss`, and rails-assets-bootstrap has
`bootstrap.scss`, so with `_bootstrap` it uses the correct bootstrap.
... this breaks the Rails 5 upgrade, and it's actually no longer needed.
New installations will have the right size anyway, and even if some
older installations miss the migration by not updating for 2 years, it
still doesn't matter since there is no risk that we will ever have
emojis in our migration filenames.
Although this is contrary to rails best-practises, we cannot provide a schema.rb that works for both MySQL and PostgreSQL, so we have no choice. Our migrations are maintained, so it should always be possible to get back to a "clean" database schema anyway.
we released that in 0.5.0.0 in 2015, we do not support skipping majors
anyway, and this is broken in Rails 5, so let's remove this. If people
upgrade from before 0.5.0.0, they have to upgrade via 0.6.0.0, but
that's written in the documenation.
Otherwise Rails 5 would ignore the migration version and create the
tables with bitints as IDs on MySQL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants