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

Implement support for Rails 5.2 #488

Merged

Conversation

erkie
Copy link
Contributor

@erkie erkie commented Apr 16, 2018

This was what I needed to be almost 1 be able to run Octopus using Rails 5.2 that was released recently. This fork can probably(maybe?) be used by anyone wanting to start using Rails 5.2, until something more official comes along.

1 I say almost because it still crashes on #477 So to be able to start using this gem I created a separate branch in my fork where I just disabled migrations for now. Currently I don't need it. A lot had changed in the Rails classes and I know too little of what should go where... https://github.com/feederco/octopus/tree/rails-5.2-disabled-migrations

@erkie
Copy link
Contributor Author

erkie commented Apr 17, 2018

I have amended this PR with a fix for this issue which seems to be related to Rails 5.2 #489

It seems like a hacky fix and I do not know what implications it might have on running processes. So see it as a basis for discussion :)

@thiagopradi thiagopradi changed the base branch from master to feature/updating-octopus-versions June 4, 2018 03:36
@thiagopradi
Copy link
Owner

Hi @erkie - I'm merging your PR into the base branch for Rails 5.2 called feature/updating-octopus-versions. I'll let you know once work is complete. Thanks for your contribution!

@thiagopradi thiagopradi merged commit 066f1fd into thiagopradi:feature/updating-octopus-versions Jun 4, 2018
def self.atleast_rails51?
ActiveRecord::VERSION::MAJOR > 5 || (ActiveRecord::VERSION::MAJOR == 5 && ActiveRecord::VERSION::MINOR >= 1)
end

def self.atleast_rails52?
ActiveRecord::VERSION::MAJOR > 5 || (ActiveRecord::VERSION::MAJOR == 5 && ActiveRecord::VERSION::MINOR >= 1)

Choose a reason for hiding this comment

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

Guys, since the method name is atleast_rails52, shouldn't it be:

ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 2
# or Fix ActiveRecord::VERSION::MINOR >= 1 to ActiveRecord::VERSION::MINOR >= 2
# because it's exactly the same as the `.atleast_rails51?` method is.

The same question for the method . atleast_rails51?...
🤔

Choose a reason for hiding this comment

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

@thiagopradi ☝️

Copy link
Owner

Choose a reason for hiding this comment

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

@bvicenzo - you are right. I'll double check and fix this method.

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.

3 participants