Skip to content

Verify database set up or throw warning #1733

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.

Sign up for GitHub

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

Merged
louispt1 merged 2 commits into from
Apr 22, 2026
Merged

Verify database set up or throw warning #1733

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15eed40
Verify database set up or throw warning
louispt1 Apr 8, 2026
0528bf6
Merge branch 'master' into db-setup
louispt1 Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions bin/setup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,18 @@ FileUtils.chdir APP_ROOT do
puts "\n== Preparing database =="
system! "bin/rails db:prepare"

puts "\n== Verifying database setup =="
verify_cmd = <<~BASH
mysql -u root -e "SELECT COUNT(*) FROM scenarios LIMIT 1;" etengine_development 2>&1 > /dev/null
BASH

if system(verify_cmd)
puts "✓ Database setup verified successfully"
else
puts "⚠ Warning: Database may not be fully set up. The scenarios table might be missing."
puts "Try running: bin/rails db:drop db:create db:schema:load db:seed"
end

Comment on lines +28 to +39
Copy link
Copy Markdown
Member

@aaccensi aaccensi Apr 20, 2026

Choose a reason for hiding this comment

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

If I understand the intent of these changes correctly, I believe this might be not the right check.

Current check only fails on completely missing table, we might be better checking a few more things, like that the migrations have run or that there is the necessary seed data. Also this assumes that mysql is installed even though that is sort of a hard requisite.

The following is not necessary the right solution, but is to illustrate where I'm going with the "right check":

puts "\n== Verifying database setup =="
if system("bin/rails runner 'ActiveRecord::Migration.check_pending!' 2>/dev/null")
  puts "✓ Database migrations are up to date"
else
  puts "⚠ Warning: Database may not be fully set up. Migrations may be pending or the schema is missing."
  puts "Try running: bin/rails db:drop db:create db:schema:load db:seed"
end

puts "\n== Verifying seed data =="
result = `bin/rails runner "puts User.where(admin: true).exists?" 2>/dev/null`.strip
if result == "true"
  puts "✓ Seed data verified successfully"
elsif result == "false"
  puts "⚠ Warning: No admin user found. Seeds may not have run."
  puts "Try running: bin/rails db:seed"
else
  puts "⚠ Warning: Could not verify seed data (database may not be reachable)."
end

Copy link
Copy Markdown
Member Author

@louispt1 louispt1 Apr 22, 2026

Choose a reason for hiding this comment

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

db:prepare won't work without mysql, so you should get a more extreme error/warning for that. The seed data is unreliable and often the migrations do not run correctly (for example because there are no scenarios in the case of etengine or no data at all in the case of etmodel).

I do think we could make this more robust by improving the seed data or being more explicit with the commands (running schema:load for example) but the point of these PRs was more a quick fix for something I saw popping up when new internal users were setting up their DBs, which is that sometimes for etmodel/etengine the databases were not created properly. Sorry that that wasn't clear in the PR text!

Copy link
Copy Markdown
Member Author

@louispt1 louispt1 Apr 22, 2026

Choose a reason for hiding this comment

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

If you think we should address this stuff more concretely let's put it on the sprint, but in the interest of MVPing it I would propose we just keep it simple. It's also a pain to test these changes as you need to drop your DB and start over 😅

puts "\n== Removing old logs and tempfiles =="
system! "bin/rails log:clear tmp:clear"

Expand Down
Loading