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

Rails 7 Support #896

Merged
merged 7 commits into from
Feb 23, 2022
Merged

Rails 7 Support #896

merged 7 commits into from
Feb 23, 2022

Conversation

tpendragon
Copy link
Collaborator

This PR is an adjustment of #892 to include general Rails 7 support.

Original message from @cgalarza :

I am working on adding Valkyrie to a fresh Rails 7/Ruby 2.7 application, but I am having trouble because Rails 7 deprecated the connection_config method available in ActiveRecord in favor of connection_db_config. In this PR, I attempt to choose the appropriate method based on what’s available.

For the specs, I wasn’t sure how best to test the ID generation. Should I specifically check that the host and database are set to the correct values instead of just checking that they are non-null values?

The pipeline fails because of coverage. With this solution it would not be possible to get 100% coverage for the connection_configuration method. What should I do?

I am pretty sure I have an individual CLA on file, is there a way to check? I am asking now if UPenn has a corporate one on file and if we just need to add my name to it.

Thanks for taking a look at this! Please let me know if there's anything I should change.

cgalarza and others added 7 commits February 22, 2022 10:14
In Rails 7, connection_config was deprecated in favor or connection_db_config.
We can't find anyone using them, they're extra code, and we can't
support them in both Rails 6 & 7.
@tpendragon
Copy link
Collaborator Author

@cgalarza Here's the PR. I added some mocking business to get around the coverage issue, can you take a look and see how it feels?

@cgalarza
Copy link
Contributor

@tpendragon Using mocking to get around the coverage issue is really clever! PR looks good to me. I also just confirmed that I have an iCLA on file and that UPenn has a cCLA on file.

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