search

juspay / services-flake

NixOS-like services for Nix flakes
https://community.flake.parts/services-flake
MIT License
342 stars 28 forks source link

fix: use absolute socket path in configureTimezones. this fixes #169 #170

Closed attilaersek closed 4 months ago

attilaersek commented 4 months ago

The change fixes #169. Tested locally. To reproduce the original issue just try to spin up a local mysql service using nixpkgs-old.legacyPackages.mysql57 package while mysqld.default-time-zone or importTimeZones is set.

attilaersek commented 4 months ago

@shivaraj-bh : thanks for your review! instead of polluting an existing test case I added two new test scenario m3 and m4, for mysql80 and mariadb respectively. I've also noticed a small cosmetics issue in mysql with the trailing slash in tz dir. Fixed that as well. If you prefer merging the test cases into an existing one, just let me know.

shivaraj-bh commented 4 months ago

If you prefer merging the test cases into an existing one, just let me know.

I don’t mind adding a newer instance of mysql, if that’s required. I think for testing mysql80 it makes sense to add a newer mysql m3, but we could merge the m4 into m2 since m2 doesn’t test anything apart from starting mariadb on a different port.

attilaersek commented 4 months ago

I've improved the tzdata tests cases to actually check whether the time_zone_name table is correctly populated or not. By default this table does not contain any row.

attilaersek commented 4 months ago

If you prefer merging the test cases into an existing one, just let me know.

I don’t mind adding a newer instance of mysql, if that’s required. I think for testing mysql80 it makes sense to add a newer mysql m3, but we could merge the m4 into m2 since m2 is doesn’t test anything apart from starting mariadb on a different port.

Sure! I'll merge m4 and m2 into one to simplify the code a bit.

shivaraj-bh commented 4 months ago

Thanks! Everything looks good at a glance. Let me do a quick test on my machine and then we are good to merge.

Meanwhile, could you do one last thing. Can you squash your commits into 2 commits? one fixing the importTimezones and another one for adding tests? We are using commit history to generate the changelogs in the future releases, so this will be very helpful.

attilaersek commented 4 months ago

Thanks! Everything looks good at a glance. Let me do a quick test on my machine and then we are good to merge.

Meanwhile, could you do one last thing. Can you squash your commits into 2 commits? one fixing the importTimezones and another one for adding tests? We are using commit history to generate the changelogs in the future releases, so this will be very helpful.

I just wanted to ask! 👍

shivaraj-bh commented 4 months ago

Awesome! Thanks for the contribution. Merging it