[DPE-4198] Switch to constant locales #559
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #559 +/- ##
==========================================
- Coverage 70.84% 70.83% -0.02%
==========================================
Files 11 12 +1
Lines 3032 3024 -8
Branches 537 535 -2
==========================================
- Hits 2148 2142 -6
+ Misses 769 768 -1
+ Partials 115 114 -1 ☔ View full report in Codecov by Sentry. |
| response_lc_monetary: Optional[Literal[tuple(SNAP_LOCALES)]] | ||
| response_lc_numeric: Optional[Literal[tuple(SNAP_LOCALES)]] | ||
| response_lc_time: Optional[Literal[tuple(SNAP_LOCALES)]] |
There was a problem hiding this comment.
I don't think we need the validator.
| @pytest.mark.group(1) | ||
| async def test_postgresql_locales(ops_test: OpsTest) -> None: | ||
| raw_locales = await run_command_on_unit( | ||
| ops_test, | ||
| ops_test.model.applications[DATABASE_APP_NAME].units[0].name, | ||
| "ls /snap/charmed-postgresql/current/usr/lib/locale", | ||
| ) | ||
| locales = raw_locales.splitlines() | ||
| locales.append("C") | ||
| locales.sort() | ||
|
|
||
| # Juju 2 has an extra empty element | ||
| if "" in locales: | ||
| locales.remove("") | ||
| assert locales == SNAP_LOCALES | ||
|
|
||
|
|
There was a problem hiding this comment.
This should fail if the snap locales change after an update, so we can regenerate the SNAP_LOCALES
There was a problem hiding this comment.
If the ls command was failing/unstable before (when running inside the leader elected hook), wouldn't this test potentially be unstable for the same underlying reason? Shouldn't we maybe retry this command in case of failure? No hard opinion here, just worrying for instabilities 😅
There was a problem hiding this comment.
If the
lscommand was failing/unstable before (when running inside the leader elected hook), wouldn't this test potentially be unstable for the same underlying reason? Shouldn't we maybe retry this command in case of failure? No hard opinion here, just worrying for instabilities 😅
The original issue happens if we try to validate the config model while the snap is not yet available/failed to install, so we can't list the locales available within the snap. If we are reaching this far into the test suite, the snap has already been installed, since the cluster has already bootstrapped.
There was a problem hiding this comment.
Now I get the timing! Sounds good then
| @@ -1429,22 +1429,35 @@ def test_validate_config_options(harness): | |||
|
|
|||
| with pytest.raises(ValueError) as e: | |||
| harness.charm._validate_config_options() | |||
| assert e.msg == "request_date_style config option has an invalid value" | |||
There was a problem hiding this comment.
The context exists when the exception is raised
src/locales.py
Outdated
There was a problem hiding this comment.
Essentially the sorted output of ls /snap/charmed-postgresql/current/usr/lib/locale + C
dragomirp
requested review from
marceloneppel,
taurus-forever and
lucasgameiroborges
There was a problem hiding this comment.
I am not happy of hardcoding the highly changeable locales file locally.
It is an extra efforts we are going to handle forever. And we will forget to update it.
Why don't we simply defer leader-elected if locale file is missing... OR use default C.UTF-8 locale if snap failed to give us /snap/charmed-postgresql/current/usr/lib/locale
IMHO, it is foreign responsibility we are workarounding for no real reason.
I don't think the locale names will change all that often and they should only change when we rebuild the snap. If we forget to update the list, the integration test check will fail, since the hardcoded list and what's in the snap will differ.
The original issue can potentially happen not only when we actually set a locale, but when we construct the validator for the locales. If |
There was a problem hiding this comment.
I personally don't think the hard-coded list will be that much trouble, the test should warn us in case of issues, so LGTM. Only semi-worried about a potentially unstable test, does the original issue came up very often? I hope not, otherwise it could be +1 unstable test 🙏
Hardcode the locales, so that there's no need to get them from the snap all the time.