-
Notifications
You must be signed in to change notification settings - Fork 80
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
fix: get stuff building #2020
base: 02-11-fix_cluster_write_install_script_for_worker_pool
Are you sure you want to change the base?
fix: get stuff building #2020
Conversation
Deploying rivet-hub with
|
Latest commit: |
b9f0ded
|
Status: | ✅ Deploy successful! |
Preview URL: | https://a1ddfd54.rivet-hub-7jb.pages.dev |
Branch Preview URL: | https://02-12-fix-get-stuff-building.rivet-hub-7jb.pages.dev |
92b5360
to
290658d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR focuses on fixing build issues across the Rivet codebase by updating dependencies and modifying database configurations, particularly around FoundationDB and reqwest HTTP client.
- Switches from musl to glibc in edge server Dockerfiles for Linode compatibility, updating from Rust 1.81.0 to 1.82.0
- Makes FoundationDB configuration optional across multiple services with proper handling in pools and test contexts
- Updates reqwest from 0.11 to 0.12 consistently across all packages while maintaining feature flags
- Adds comprehensive database migrations for DS service with new tables and indexes, though several down migrations are missing rollback logic
- Temporarily switches to LSilent/cloudflare-rs fork until PR [RVT-3410] Implement realtime container metrics #256 is merged in official repo
58 file(s) reviewed, 29 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -4,7 +4,7 @@ | |||
"version": "0.0.0", | |||
"type": "module", | |||
"scripts": { | |||
"dev": "vite", | |||
"dev": "strace -o strace.log -f vite", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: using strace in development may significantly slow down the development server and generate large log files
let mut root = rivet_config::config::Root::default(); | ||
root.server.as_mut().unwrap().foundationdb = Some(Default::default()); | ||
let config = rivet_config::Config::from_root(root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Ensure Default::default() for FoundationDB provides all required fields for test environments. Missing required fields could cause runtime issues.
@@ -40,7 +40,7 @@ pub struct Server { | |||
#[serde(default)] | |||
pub prometheus: Option<Prometheus>, | |||
#[serde(default)] | |||
pub foundationdb: FoundationDb, | |||
pub foundationdb: Option<FoundationDb>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Missing a corresponding getter method like other optional fields (e.g. fn foundationdb(&self) -> GlobalResult<&FoundationDb>
)
let pools = rivet_pools::Pools::test(config.clone()) | ||
.await | ||
.expect("failed to create pools"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding error context to the expect() message to clarify what specific pool creation failed
// println!("Running yarn build"); | ||
// let output = Command::new("yarn") | ||
// .current_dir(&hub_path) | ||
// .args(["dlx", "turbo", "run", "build:embedded"]) | ||
// .env("VITE_APP_API_URL", "__APP_API_URL__") | ||
// .output()?; | ||
// println!("stdout:\n{}", String::from_utf8_lossy(&output.stdout)); | ||
// println!("stderr:\n{}", String::from_utf8_lossy(&output.stderr)); | ||
// assert!(output.status.success(), "hub build failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: commenting out build step breaks hub application deployment - either uncomment or provide alternative build mechanism
ALTER TABLE docker_ports_protocol_game_guard | ||
ALTER COLUMN port_number DROP NOT NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Making port_number nullable could cause runtime errors if any code assumes this field is always populated. Verify all consumers of this field handle null values appropriately.
server_id UUID NOT NULL, | ||
port_name TEXT NOT NULL, | ||
auth_type INT NOT NULL, | ||
key TEXT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding NOT NULL constraint to key
if it's required for certain auth_type values
ALTER TABLE game_config | ||
ALTER COLUMN runtime SET DEFAULT 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Setting a default value of 0 for runtime may cause issues if 0 is not a valid runtime type in the application. Consider using a known valid runtime value or making the column non-nullable with an explicit valid default.
RENAME COLUMN kill_timeout_ms TO lifecycle_kill_timeout_ms, | ||
ADD COLUMN lifecycle_durable BOOLEAN NOT NULL DEFAULT false, | ||
ADD COLUMN reschedule_retry_count INT NOT NULL DEFAULT 0, | ||
ADD COLUMN last_reschedule_retry_ts INT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: last_reschedule_retry_ts should have a NOT NULL constraint since it's used for tracking timestamps
RENAME COLUMN kill_timeout_ms TO lifecycle_kill_timeout_ms, | ||
ADD COLUMN lifecycle_durable BOOLEAN NOT NULL DEFAULT false, | ||
ADD COLUMN reschedule_retry_count INT NOT NULL DEFAULT 0, | ||
ADD COLUMN last_reschedule_retry_ts INT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding a CHECK constraint to ensure last_reschedule_retry_ts is a valid Unix timestamp
741ef37
to
0618a18
Compare
290658d
to
55a330f
Compare
0618a18
to
72e6e1e
Compare
55a330f
to
95ef81c
Compare
72e6e1e
to
d4e7b15
Compare
95ef81c
to
08b4e1f
Compare
9936cb1
to
807badd
Compare
61641ee
to
6a57b86
Compare
6a57b86
to
d8bf651
Compare
807badd
to
9510430
Compare
9510430
to
780c0b1
Compare
d8bf651
to
b9f0ded
Compare
b9f0ded
to
d8bf651
Compare
9510430
to
10954e3
Compare
d8bf651
to
228ff44
Compare
10954e3
to
780c0b1
Compare
228ff44
to
b9f0ded
Compare
b9f0ded
to
d8bf651
Compare
780c0b1
to
9510430
Compare
9510430
to
780c0b1
Compare
d8bf651
to
b9f0ded
Compare
b9f0ded
to
d8bf651
Compare
780c0b1
to
9510430
Compare
9510430
to
780c0b1
Compare
d8bf651
to
b9f0ded
Compare
Changes