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

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

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

attilaersek
Copy link
Contributor

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.

Copy link
Member

@shivaraj-bh shivaraj-bh 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.

Could you also include a test for this by enabling importTimeZones in either mysql.m1 or mysql.m2 here: https://github.com/juspay/services-flake/blob/main/nix/mysql/mysql_test.nix?

@attilaersek
Copy link
Contributor Author

@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
Copy link
Member

shivaraj-bh commented Apr 24, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

shivaraj-bh commented Apr 24, 2024

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
Copy link
Contributor Author

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
Copy link
Member

Awesome! Thanks for the contribution. Merging it

@shivaraj-bh shivaraj-bh merged commit b5d29b0 into juspay:main Apr 24, 2024
4 checks passed
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.

Could not create unix socket lock file error in mysql service when timezones are imported
2 participants