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

Deps: Rails 8 #959

Merged
merged 7 commits into from
Mar 7, 2025
Merged

Deps: Rails 8 #959

merged 7 commits into from
Mar 7, 2025

Conversation

ericenns
Copy link
Member

@ericenns ericenns commented Feb 20, 2025

What does this PR do and why?

Describe in detail what your merge request does and why.

This PR upgrades our rails dependency to 8.0.1.

A few notables changes:

  • Removed *DATABASE_URL env variables from config/database.yml
  • Changed identifier in rubyLsp.rubyVersionManager to auto, so that a developer doesn't need asdf if they don't want to use it (e.g. to support devcontainers better in the future)
  • Dockerfile updated to match one generated by rails
  • bin/docker-entrypoint updated to support using thruster (no change to use it yet though)
  • Added bin/setup which is necessary for devcontainers
  • Updated to use Rails 8.0 config defaults
  • Changed logging to default to STDOUT, no file based logging for production (we already use STDOUT based logging with container deploy)
  • Updated Capybara config to make it compatible for devcontainer later on

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other pull requests.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Ensure the tests pass
  2. Ensure that the application is working as expected

PR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

This comment has been minimized.

This comment has been minimized.

@ericenns ericenns marked this pull request as ready for review February 21, 2025 13:35
@ericenns ericenns requested a review from deepsidhu85 February 21, 2025 13:35

This comment has been minimized.

deepsidhu85
deepsidhu85 previously approved these changes Mar 6, 2025
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

This looks good to me and works as expected. Tested it out with local storage, and with azurite (minus the workflow executions as sapporo nextflow doesn't recognize azurite)

Copy link
Member

@JeffreyThiessen JeffreyThiessen left a comment

Choose a reason for hiding this comment

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

Looks good. Tested a bunch of graphql queries/mutations as well as web ui functionality and everything seems to be working as expected.

Please update the schema files db/schema.rb and db/jobs_schema.rb which should have been updated to schema 8.0 and have enable_extension "pg_catalog.plpgsql"

ChrisHuynh333
ChrisHuynh333 previously approved these changes Mar 6, 2025
Copy link
Collaborator

@ChrisHuynh333 ChrisHuynh333 left a comment

Choose a reason for hiding this comment

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

Looks good!

@ericenns ericenns dismissed stale reviews from ChrisHuynh333 and deepsidhu85 via 89b8cde March 7, 2025 13:23
@ericenns
Copy link
Member Author

ericenns commented Mar 7, 2025

pg_catalog.plpgsql

I've ran bin/rails db:drop db:create db:migrate in 89b8cde which update both schemas.

Copy link

github-actions bot commented Mar 7, 2025

Code Metrics Report

Coverage Test Execution Time
94.7% 10m58s

Reported by octocov

Copy link
Member

@JeffreyThiessen JeffreyThiessen left a comment

Choose a reason for hiding this comment

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

Looks great!

@deepsidhu85 deepsidhu85 merged commit 2c0c337 into main Mar 7, 2025
4 of 5 checks passed
@deepsidhu85 deepsidhu85 deleted the rails-8 branch March 7, 2025 16:05
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.

4 participants