-
Notifications
You must be signed in to change notification settings - Fork 50
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
chore: Upgrade to Diesel 2.2 #1647
base: master
Are you sure you want to change the base?
Conversation
See https://diesel.rs/guides/migration_guide.html. - This pulls in mysqlclient-sys >=0.4.0, which allows using libmariadb instead of libmysqlclient. That allows using Debian's bundled client library. MySQL's APT repository only has packages for AMD64. - Connections are now passed as mut refs, which requires RefCell/RwLock. - Embedded migrations had a slight API change. - put_bso_sync was calling update_collection without its transaction, which now causes a deadlock. Updated to write inside its transaction. Signed-off-by: Tommie Gannert <[email protected]>
Signed-off-by: Tommie Gannert <[email protected]>
Signed-off-by: Tommie Gannert <[email protected]>
The global functions were replaced with Server in 0.32.0 and the Cargo.toml has 1.4 listed (probably due to a mistake when pinning Diesel at 1.4.) Signed-off-by: Tommie Gannert <[email protected]>
Cargo audit: Crate: idna Version: 0.5.0 Title: `idna` accepts Punycode labels that do not produce any non-ASCII when decoded Date: 2024-12-09 ID: RUSTSEC-2024-0421 URL: https://rustsec.org/advisories/RUSTSEC-2024-0421 Solution: Upgrade to >=1.0.0 Signed-off-by: Tommie Gannert <[email protected]>
There is no other way for the timestamp to increase in production. Signed-off-by: Tommie Gannert <[email protected]>
Since the batch_uploads primary key already contains user_id, it does nothing to increase time resolution. The counter may still see conflicts with other processes, but the probably is much lower now. (Spanner uses UUIDv4 for batch IDs.) The user_id hack was added in e9b455f "to match the Python implementation". E2E tests are failing because we can now generate more than one batch within 10 ms. Signed-off-by: Tommie Gannert <[email protected]>
Ugh, Looks like these commits aren't signed, so this PR can't be merged. You can tell because the commits don't have a You can sign using the same SSH key that you set up in github, which can be easier (and more secure) than dealing with GPG signatures.
That's basically what I'd do to duplicate your PR as signed. |
Oh, you actually did mean a digital signature. Now I'm curious what this gains you? The code merged is what's reviewed in the PR, so it doesn't seem to protect against anything? |
It's required by our Security Operations (SecOps) folk for all production run code. Basically, it's to ensure that this and future code comes from "you", and can be trusted. This article presents a fairly good reason for why we require them, even though we're open source and public. |
beh, So, in these cases I usually just pull master, drop I'll approve after that, but I'll want to give at least one other team member a chance before I land anything. |
Description
Upgrades Diesel to 2.2.
This uncovered some bugs in the MySQL backend, e.g. there can be at most one batch ID per user every 10 ms. It seems Diesel 2 is faster (and libmysqlclient is faster than libmariadb), causing the E2E batch tests to generate batches faster than before. Nothing ever updated the session timestamp in the backend, which suggests there could only really be one batch per user.
Split off from #1644.
Testing
Existing integration tests.
Issue(s)
Closes SYNC-4568 (?)