mypy: Add type hints to pglookout [BF-1560] #106

Mulugruntz · 2023-03-23T15:03:40Z

Important Note

DO NOT merge until we test this on aiven-core pipeline

This can be tracked there (private repo):

https://github.com/aiven/aiven-core/pull/32518

Removed python2 leftovers (from pglookout 2.0.0 (2018-10-12))
- In pglokout/debian/rules;
- In setup.py.
Removed most of manual path handling through os and string manipulation to replace them with pathlib.Path.
Unit tests:
- More unit tests in test/test_common.py;
- Improve test/conftest.py to better leverage pytest features.
- In general, removed some deprecated pytest features (i.e. tmpdir).
In README.rst:
- Alphabetically sort configuration keys. This has the effect of grouping related keys closer.
- Add missing key: replication_catchup_timeout
In pglookout/pglookout.py:
- Refactor check_cluster_state to simplify the branching of a part of this method.
  This helps mypy and makes it clearer about what the different preconditions are.
- Rename its test file from test/test_lookout.py -> test/test_pglookout.py to follow conventions.
- New method to normalize config. When loading the config, a conversion is done to only have the
  dict-style type in the config's remote_conns. This simplifies the preconditions.
In pglookout/config.py:
- New file to type hint the configuration. This can be used by developers to check their code.

Also in this commit: - removed `python2` leftovers (from pglookout 2.0.0 (2018-10-12))

Also in this commit: - More unit tests; - Removed redundant `()` in regex.

New file to type hint the configuration. Also: - Alphabetically sort configuration keys in the `README`. This has the effect of grouping related keys closer. - Add missing key: `replication_catchup_timeout`

This refactor is made to simplify the branching of a part of this method. This helps `mypy` and makes it clearer about what the different preconditions.

Rename `test_lookout.py` -> `test_pglookout.py` to follow conventions.

Change `remote_conns` type hint. The temporary `str` type is no longer accepted. When loading the config, a conversion is done to only have the dict-style type in the config's `remote_conns`.

Extraction of the 3 read functions: from file, git, and Makefile.

Package https://pypi.org/project/futures/ is not needed in Python3.

Also includes some cleanup to better leverage pytest features.

codecov-commenter · 2023-03-27T16:20:08Z

Codecov Report

Patch coverage: 81.50% and project coverage change: +3.11 🎉

Comparison is base (94891fa) 68.20% compared to head (dac0aad) 71.32%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   68.20%   71.32%   +3.11%     
==========================================
  Files          10       13       +3     
  Lines         975     1203     +228     
  Branches      171      182      +11     
==========================================
+ Hits          665      858     +193     
- Misses        253      280      +27     
- Partials       57       65       +8

Impacted Files	Coverage Δ
pglookout/current_master.py	`0.00% <0.00%> (ø)`
version.py	`100.00% <ø> (ø)`
pglookout/logutil.py	`52.94% <60.00%> (-4.76%)`	⬇️
pglookout/cluster_monitor.py	`64.82% <75.00%> (+0.31%)`	⬆️
pglookout/pglookout.py	`67.60% <75.19%> (-1.24%)`	⬇️
pglookout/common.py	`100.00% <100.00%> (ø)`
pglookout/common_types.py	`100.00% <100.00%> (ø)`
pglookout/config.py	`100.00% <100.00%> (ø)`
pglookout/default.py	`100.00% <100.00%> (ø)`
pglookout/pgutil.py	`95.76% <100.00%> (+3.33%)`	⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

This fixes some behaviour discrepancy between local and CI.

Mulugruntz · 2023-03-27T15:20:25Z

README.rst

@@ -201,6 +201,11 @@ description of the current state of the cluster which is under monitoring.
 Configuration keys
 ==================

+``alert_file_dir`` (default ``os.getcwd()``)


I alphabetically sorted configuration keys in the README. This has the effect of grouping related keys closer.

Maybe a different PR? This one is already big enough:)

The sort could have been in a different PR or a different commit (just the sort operation, no change). My bad 🙇.
Having them in 016d14d might not be the best. That's why I added the GitHub comments to guide reviewers (there's a sort and there's one key that was missing). If that's not good enough, sure it would be possible to split, rebase interactively (pray), force push, modify the test in aiven-core to target the new hash. Tell me if you want this.

Mulugruntz · 2023-03-27T15:20:59Z

README.rst

+``replication_catchup_timeout`` (default ``300.0``)
+
+How long should pglookout wait for a node to catch up with the primary
+before it decides to promote itself.


Added missing key: replication_catchup_timeout

Mulugruntz · 2023-03-28T08:14:31Z

setup.py

@@ -18,9 +17,6 @@
    "requests >= 1.2.0",
 ]

-if sys.version_info[0] == 2:


Package https://pypi.org/project/futures/ is not needed in Python3.

Mulugruntz · 2023-03-28T08:15:57Z

test/conftest.py

-def db():
-    tmpdir_obj = py_path.local(tempfile.mkdtemp(prefix="pglookout_dbtest_"))
-    tmpdir = str(tmpdir_obj)
+def db(tmp_path_factory: pytest.TempPathFactory) -> Generator[TestPG, None, None]:


Now, we can use tmp_path_factory for session scoped fixtures. So I simplified this fixture.

Mulugruntz · 2023-03-28T08:18:51Z

pglookout/pglookout.py

+        if "remote_conns" in config:
+            config["remote_conns"] = {name: get_connection_info(info) for name, info in config["remote_conns"].items()}


New method to normalize config. When loading the config, a conversion is done to only have the dict-style type in the config's remote_conns. This simplifies the preconditions.
We could extend this method further to validate more of the config, in the future. Or use better libraries to delegate that part (pydantic).

Mulugruntz · 2023-03-28T10:00:39Z

pglookout/default.py

+MISSING_MASTER_FROM_CONFIG_TIMEOUT: Final[float] = 15.0
+MAINTENANCE_MODE_FILE: Final[str] = "/tmp/pglookout_maintenance_mode_file"
+PG_DATA_DIRECTORY: Final[str] = "/var/lib/pgsql/data"
+JSON_STATE_FILE_PATH: Final[str] = "/tmp/pglookout_state.json"


These values are extracted from the implementation, in order to guarantee that there is no change in behaviour.
More work could be done, there's still plenty of magic constants.

Mulugruntz · 2023-03-28T10:03:16Z

pglookout/logutil.py

+    if isinstance(facility, str):
+        facility_id: int = SysLogHandler.facility_names.get(facility, SysLogHandler.LOG_LOCAL2)
+    else:
+        facility_id = facility


A change that is not really a change. Our README mentions:

``syslog_facility`` (default ``"local2"``) Determines syslog log facility. (requires syslog to be true as well)

Mulugruntz · 2023-03-28T10:05:33Z

pglookout/pglookout.py

-        signal.signal(signal.SIGHUP, self.load_config)
-        signal.signal(signal.SIGINT, self.quit)
-        signal.signal(signal.SIGTERM, self.quit)
+        signal(SIGHUP, self.load_config_from_signal)


New method, see below.

Mulugruntz · 2023-03-28T10:06:57Z

pglookout/pglookout.py

+        if not self._is_initial_state_valid():
+            self.log.error("Initial state is invalid, exiting.")
+            sys.exit(1)


Added this check to fail early. Ideally, we wouldn't need that. But there are some tricky states that we want to avoid. Read the method for more info (a few lines below).

Mulugruntz · 2023-03-28T10:09:48Z

pglookout/pglookout.py

+    def load_config_from_signal(self, _signal: int, _frame: FrameType | None = None) -> None:
        self.log.debug(
            "Loading JSON config from: %r, signal: %r, frame: %r",
            self.config_path,
            _signal,
            _frame,
        )
+        self.load_config()


This new method is the only one expected to have access to signal and frame. This type of info should not reach the load_config. The behaviour is left unchanged.

However, now, that means that load_config can be called as a normal method.

pglookout.spec

alanfranz

This PR contains many different changes.

It's true that, sometimes,when adding types you must change some code; but my impression is that this sneaks many small changes along with typing. The combined effect of all those changes isn't really clear to me.

Since this is a critical component, we should make each change more isolated, and not introduce semantic changes in a commit with "mypy" in the message, unless they're absolutely needed to make the code work (sometimes when using mypy we realize there's a "dynamic typing" magic going on) or to properly parse validated data structures, if that's the case (but, if today it works even when not validated because of some type coercion or magic string conversion, changing that could still be an issue).

I still suggest you first try this in aiven-core btw. If it works and passes all tests, that would be a great indicator that things should work properly, and we can think about how to restructure/split this pr.

pglookout/logutil.py

pglookout/pgutil.py

alanfranz · 2023-04-04T13:17:58Z

pglookout/webserver.py

 class ThreadedWebServer(ThreadingMixIn, HTTPServer):
-    cluster_state = None
-    log = None
-    cluster_monitor_check_queue = None
-    allow_reuse_address = True
+    allow_reuse_address: bool = True
+
+    def __init__(
+        self,
+        address: str,
+        port: int,
+        RequestHandlerClass: type[BaseHTTPRequestHandler],
+        cluster_state: dict[str, MemberState],
+        log: Logger,
+        cluster_monitor_check_queue: Queue[str],
+    ) -> None:
+        super().__init__((address, port), RequestHandlerClass)
+        self.cluster_state: dict[str, MemberState] = cluster_state
+        self.log: Logger = log
+        self.cluster_monitor_check_queue: Queue[str] = cluster_monitor_check_queue


I'd not mix this change with typing changes. And, this comment should be in the commit message anyway.

alanfranz · 2023-04-04T13:18:32Z

pglookout/webserver.py

+        self.server = ThreadedWebServer(
+            address=self.address,
+            port=self.port,
+            RequestHandlerClass=RequestHandler,
+            cluster_state=self.cluster_state,
+            log=self.log,
+            cluster_monitor_check_queue=self.cluster_monitor_check_queue,
+        )


Nevertheless, I'd keep this in a separate commit with its own message.

pglookout/webserver.py

alanfranz · 2023-04-04T13:46:26Z

pglookout/pglookout.py

+        if not self.own_db:
+            self._log_state_when_own_db_is_not_in_cluster(
+                observer_state=observer_state,
+                standby_nodes=standby_nodes,
+            )
+        else:
+            assert own_state is not None
+            consider_failover = self._is_failover_consideration_needed(
+                own_state=own_state,
+                observer_state=observer_state,
+                standby_nodes=standby_nodes,
+                master_node=master_node,
+            )

-        if self.own_db:
-            if self.own_db == self.current_master:
-                # We are the master of this cluster, nothing to do
-                self.log.debug(
-                    "We %r: %r are still the master node: %r of this cluster, nothing to do.",
-                    self.own_db,
-                    own_state,
-                    master_node,
-                )
-                return
-            if not standby_nodes:
-                self.log.warning("No standby nodes set, master node: %r", master_node)
-                return
-            self.consider_failover(own_state, master_node, standby_nodes)
+        if consider_failover and own_state:
+            self.consider_failover(
+                own_state=own_state,
+                master_node=master_node,
+                standby_nodes=standby_nodes,
+            )


Didn't review this part.

This makes sure that the tests can run in their own environment, without affecting the system deps, while still using the system deps.

…type_hints # Conflicts: # pglookout/pglookout.py # test/test_pglookout.py

Alan is currently OoO.

kathia-barahona

I haven't finished yet. Some of my comments can be ignore but others will be nice to revisit. Really appreciate that everything was split into different commits, but from a reviewer perspective it is a bit overwhelming and some of the changes might be behavioral and non-related to the PR (I only noticed this by navigating into each commit and actually focusing, not ideal in my opinion). I guess in those situations where that dependency cannot be avoided, is better to open a different PR. (Maybe is just me 🤷‍♀️ )

(More for me, I still need to continue from 6ce993e and further)

kathia-barahona · 2023-04-20T12:22:15Z

pglookout/statsd.py

+StatsdMetricType = Literal[
+    b"g",  # gauge
+    b"c",  # counter
+    b"s",  # set
+    b"ms",  # timing
+    b"h",  # histogram
+    b"d",  # distribution
+]


question Why not using enumerations?

This is checked at static type checking time (when running mypy) and is ignored at runtime.

An enumeration is an actual runtime "thing" (type+value). I don't want to change anything in these PRs (or as little as possible).

kathia-barahona · 2023-04-20T12:27:53Z

test/test_common.py

+@pytest.mark.parametrize(
+    "timestamp",
+    [
+        datetime(2021, 1, 1, 23, 42, 11, 123456),
+        datetime(2021, 1, 1, 23, 42, 11),
+        datetime(2021, 1, 1, 23, 42),
+        datetime(2021, 1, 1, 23),
+        datetime(2021, 1, 1),
+    ],
+)
+def test_roundtrip(timestamp: datetime) -> None:
+    ts2 = parse_iso_datetime(get_iso_timestamp(timestamp))
+
+    assert ts2 == timestamp
+
+
+@pytest.mark.parametrize(
+    ("value", "normalized_value"),
+    # fmt: off
+    [
+        # Extended format
+        ("2021-01-01T00:00:00.000000Z",     "2021-01-01T00:00:00Z"),         # noqa: E241
+        ("2021-01-01T23:42:11.123456Z",     "2021-01-01T23:42:11.123456Z"),  # noqa: E241
+        ("2021-01-01T00:00:00Z",            "2021-01-01T00:00:00Z"),         # noqa: E241
+        ("2021-01-01T23:42:11Z",            "2021-01-01T23:42:11Z"),         # noqa: E241
+        ("2021-01-01T00:00Z",               "2021-01-01T00:00:00Z"),         # noqa: E241
+        ("2021-01-01T23:42Z",               "2021-01-01T23:42:00Z"),         # noqa: E241
+        ("2021-01-01",                      "2021-01-01T00:00:00Z"),         # noqa: E241
+        # Basic format
+        ("20210101T000000Z",                "2021-01-01T00:00:00Z"),         # noqa: E241
+        ("20210101T234211123456Z",          "2021-01-01T23:42:11.123456Z"),  # noqa: E241
+        ("20210101T000000Z",                "2021-01-01T00:00:00Z"),         # noqa: E241
+        ("20210101T234211Z",                "2021-01-01T23:42:11Z"),         # noqa: E241
+        ("20210101T0000Z",                  "2021-01-01T00:00:00Z"),         # noqa: E241
+        ("20210101T2342Z",                  "2021-01-01T23:42:00Z"),         # noqa: E241
+        ("20210101",                        "2021-01-01T00:00:00Z"),         # noqa: E241
+    ],
+    # fmt: on
+)
+def test_reverse_roundtrip(value: str, normalized_value: str) -> None:
+    v2 = get_iso_timestamp(parse_iso_datetime(value))
+
+    assert v2 == normalized_value


suggestion Maybe a different PR? its sort of non-related to type hints

This is because there were some minute changes in parse_iso_datetime and get_iso_timestamp. And when having function + invert function, we should have roundtrip tests. This was missing and helped to make sure I didn't break anything. Also, I think it helps reviewers by giving higher guarantees that my changes didn't break anything.

But true, it's not a test-focused PR. So, I didn't add much.

kathia-barahona · 2023-04-20T12:29:51Z

README.rst

@@ -201,6 +201,11 @@ description of the current state of the cluster which is under monitoring.
 Configuration keys
 ==================

+``alert_file_dir`` (default ``os.getcwd()``)


Maybe a different PR? This one is already big enough:)

kathia-barahona · 2023-04-20T12:35:12Z

pglookout/config.py

+from typing import Literal, TypedDict
+
+
+class Statsd(TypedDict, total=False):


question why not total=True? isn't it important for the typed dicts to have all these keys? feels like this might lead to bugs if we are not strict enough

From what I remember, it might be because of this:

stats: Statsd = self.config.get("statsd", {}) self.stats = StatsClient(**stats)

The default has to also be a valid Statsd, and an empty dict is only valid when total=False.
Also we have this:

class StatsClient: def __init__( self, host: str | None = "127.0.0.1", port: int = 8125, tags: dict[str, str] | None = None, ) -> None: ...

where they all have a default. Therefore, they're optional. These defaults only make sense if we allow for users to provide an "incomplete" Statsd.

kathia-barahona · 2023-04-20T12:36:40Z

pglookout/config.py

+    http_port: int
+    json_state_file_path: str
+    known_gone_nodes: list[str]
+    log_level: Literal["NOTSET", "DEBUG", "INFO", "WARNING", "WARN", "ERROR", "FATAL", "CRITICAL"]


suggestion Maybe an enum?:) ig I like enums

Same reason as the other Literal vs. Enum comment. :-)

kathia-barahona · 2023-04-20T12:41:52Z

pglookout/cluster_monitor.py

-        failover_decision_queue,
-        is_replication_lag_over_warning_limit,
-        stats,
+        config: Config,


Why not doing the same for StatsClient.__init__ ? https://github.com/aiven/pglookout/blob/2ce13ab9002303040ea44cbc4f11fa6bb65722bf/pglookout/statsd.py#L32-L34
There is already Statsd https://github.com/aiven/pglookout/blob/016d14db3722dd1002a765112672afb0ba3f2d09/pglookout/config.py#L7

kathia-barahona · 2023-04-20T12:47:32Z

pglookout/common_types.py

+    state_data: str
+
+
+class MemberState(TypedDict, total=False):


I think the naming is not clear enough, I had to jump to a different commit and file just to read the docstring in order to know what was going on in 9b4e13f

Suggested change

class MemberState(TypedDict, total=False):

class ClusterMemberState(TypeDict, total=False):

Hmmmm... even with the docstring? I personally think that your name suggestion is also good and I think it makes sense. I'm just wondering.

kathia-barahona · 2023-04-20T12:51:15Z

pglookout/common_types.py

+# Note for future improvements:
+# If we want ObserverState to accept arbitrary keys, we have three choices:
+# - Use a different type (pydantic, dataclasses, etc.)
+# - Use a TypedDict for static keys (connection, fetch_time) and a sub-dict for
+#   dynamic keys (received from state.json).
+# - Wait for something like `allow_extra` to be implemented into TypedDict (unlikely)
+#   https://github.com/python/mypy/issues/4617
+ObserverState = Dict[str, Any]


This is actually confusing... ClusterMonitor.observer_state is actually dict[str, ObserverState], while ClusterMonitor._fetch_observer_state return type is Optional[ObserverState] ... so its quite misleading in my opinion. I rather see a more specific typing and later work on improvements

Yes, I pulled a lot of hair on that one. The names "observer state" were loosely used for different things. Both for an "observed state" and for an "observer state".

For now, I'd say to ignore this, as it gets improved in a later commit :-). Ideally, I could also change the name instance variable ClusterMonitor.observer_state and such, but I didn't (I think), as I didn't want to change any of the public APIs (symbols) in this PR.

kathia-barahona · 2023-04-20T12:53:49Z

pglookout/pglookout.py

+        standby_nodes: dict[str, MemberState],
+        master_node: MemberState | None,
+    ) -> None:
+        assert self.own_db is not None


I don't understand this assert. Seems like we are considering the case where self.own_db might be None or empty (missing typing?). This quite changes current behavior, a separate PR will be better for reviewing in such case

https://github.com/aiven/pglookout/blob/6ce993ed4d63e1eedd8a87d33dd37fd4562ebd05/pglookout/pglookout.py#L553-L565

This is for mypy. We should always remember that our Python code could skip asserts with -O / -OO ( https://docs.python.org/3/tutorial/modules.html#compiled-python-files ) and logic cannot rely on them.

The reason why this was added here is because mypy highlighted some preconditions. And this one here is one of them. If self.own_db is None, we would crash a few ops later (with the code currently in main). The only difference here is that

we fail with an AssertionError instead of a different type of exception (which would be much more obscure).

mypy helps

Optimized code will still behave the same as before. Unoptimized code will be cleaner.

If I don't change that, there are so many variables in this scope that rely and depend on the state of other variables outside of their scope and it's so dirty that mypy is unable to do any infer. That's why the stuff here was split into several well-scoped methods and don't depend of anything outside of their scope anymore.

maybe add a comment to it, smth like:

Suggested change

assert self.own_db is not None

assert self.own_db is not None # help mypy

kathia-barahona · 2023-04-20T12:59:39Z

pglookout/pglookout.py

+        if not self.own_db:
+            self._log_state_when_own_db_is_not_in_cluster(
+                observer_state=observer_state,
+                standby_nodes=standby_nodes,
+            )
+        else:
+            assert own_state is not None
+            consider_failover = self._is_failover_consideration_needed(
+                own_state=own_state,
+                observer_state=observer_state,
+                standby_nodes=standby_nodes,
+                master_node=master_node,
+            )

-        if self.own_db:
-            if self.own_db == self.current_master:
-                # We are the master of this cluster, nothing to do
-                self.log.debug(
-                    "We %r: %r are still the master node: %r of this cluster, nothing to do.",
-                    self.own_db,
-                    own_state,
-                    master_node,
-                )
-                return
-            if not standby_nodes:
-                self.log.warning("No standby nodes set, master node: %r", master_node)
-                return
-            self.consider_failover(own_state, master_node, standby_nodes)
+        if consider_failover and own_state:
+            self.consider_failover(
+                own_state=own_state,
+                master_node=master_node,
+                standby_nodes=standby_nodes,
+            )


As I mentioned on my previous comment, I think we should just focus on typing and later on behavioral/cosmetic changes. It currently feels this is out of scope with this PR.

alanfranz · 2023-04-28T23:52:01Z

I'm not going to review this further; consider all my comments as resolved, and handle this within team BF :-)

Mulugruntz · 2023-05-01T08:15:31Z

I'm not going to review this further; consider all my comments as resolved, and handle this within team BF :-)

Thanks @alanfranz and good luck in your new team! I'll remove you from the reviewers :-)
Edit: I don't have the rights to modify reviewers... apparently 🤔

Samuel Giffard added 5 commits March 23, 2023 16:01

mypy: pglookout/logutil.py [BF-1560]

0d4ea2c

mypy: pglookout/pgutil.py [BF-1560]

238eb76

mypy: pglookout/current-master.py [BF-1560]

3db5684

Also in this commit: - removed `python2` leftovers (from pglookout 2.0.0 (2018-10-12))

mypy: pglookout/statsd.py [BF-1560]

2ce13ab

mypy: pglookout/common.py [BF-1560]

25617d1

Also in this commit: - More unit tests; - Removed redundant `()` in regex.

Mulugruntz added the enhancement label Mar 23, 2023

Mulugruntz self-assigned this Mar 23, 2023

Samuel Giffard added 4 commits March 23, 2023 17:06

mypy: pglookout/config.py [BF-1560]

016d14d

New file to type hint the configuration. Also: - Alphabetically sort configuration keys in the `README`. This has the effect of grouping related keys closer. - Add missing key: `replication_catchup_timeout`

mypy: pglookout/common_types.py [BF-1560]

bf067b3

mypy: pglookout/webserver.py [BF-1560]

a462bf9

mypy: pglookout/cluster_monitor.py [BF-1560]

9b4e13f

Mulugruntz force-pushed the sgiffard/BF-1560_add_type_hints branch from 9d90d78 to 9b4e13f Compare March 23, 2023 16:09

Samuel Giffard added 2 commits March 24, 2023 19:00

mypy: pglookout/pglookout.py [BF-1560]

45d413b

mypy: Refactor check_cluster_state [BF-1560]

6ce993e

This refactor is made to simplify the branching of a part of this method. This helps `mypy` and makes it clearer about what the different preconditions.

Mulugruntz force-pushed the sgiffard/BF-1560_add_type_hints branch from d0304b6 to 6ce993e Compare March 24, 2023 18:02

Samuel Giffard added 7 commits March 27, 2023 17:03

mypy: test/test_pglookout.py [BF-1560]

0d438e9

Rename `test_lookout.py` -> `test_pglookout.py` to follow conventions.

mypy: New method to normalize config [BF-1560]

1f47155

Change `remote_conns` type hint. The temporary `str` type is no longer accepted. When loading the config, a conversion is done to only have the dict-style type in the config's `remote_conns`.

mypy: version.py [BF-1560]

5d4ac6e

Extraction of the 3 read functions: from file, git, and Makefile.

mypy: setup.py [BF-1560]

51bb85e

mypy: Remove python2 dependency [BF-1560]

e9834d4

Package https://pypi.org/project/futures/ is not needed in Python3.

mypy: conftest.py [BF-1560]

a3ad6f8

Also includes some cleanup to better leverage pytest features.

Bump Makefile version to 2.1.0 [BF-1560]

7434873

Mulugruntz force-pushed the sgiffard/BF-1560_add_type_hints branch from 7ba4841 to 7434873 Compare March 27, 2023 15:06

mypy: Compatible with Python 3.9 logging [BF-1560]

30bcd0d

Mulugruntz force-pushed the sgiffard/BF-1560_add_type_hints branch from 9dfaebe to 7306a03 Compare March 27, 2023 16:13

pylint: Enable more messages [BF-1560]

e4e771a

Mulugruntz force-pushed the sgiffard/BF-1560_add_type_hints branch from 7306a03 to e4e771a Compare March 27, 2023 16:15

Mulugruntz changed the title ~~mypy: add type hints to pglookout [BF-1560]~~ mypy: Add type hints to pglookout [BF-1560] Mar 28, 2023

Mulugruntz marked this pull request as ready for review March 28, 2023 08:13

Mulugruntz requested a review from a team as a code owner March 28, 2023 08:13

Samuel Giffard added 2 commits March 28, 2023 12:10

pylint: Fix broad exception raised [BF-1560]

56c01a8

pylint: Dev requirements are now pinned [BF-1560]

09a15b1

This fixes some behaviour discrepancy between local and CI.

Mulugruntz commented Mar 28, 2023

View reviewed changes

alanfranz self-assigned this Apr 4, 2023

alexole reviewed Apr 4, 2023

View reviewed changes

pglookout.spec Outdated Show resolved Hide resolved

alanfranz previously requested changes Apr 4, 2023

View reviewed changes

Mulugruntz force-pushed the sgiffard/BF-1560_add_type_hints branch from 56864e7 to 432800b Compare April 4, 2023 14:43

make: Dev reqs are now in isolation [BF-1560]

9a0abae

This makes sure that the tests can run in their own environment, without affecting the system deps, while still using the system deps.

Mulugruntz force-pushed the sgiffard/BF-1560_add_type_hints branch from 432800b to 9a0abae Compare April 4, 2023 14:50

Mulugruntz requested a review from alanfranz April 4, 2023 14:52

Merge remote-tracking branch 'origin/main' into sgiffard/BF-1560_add_…

dac0aad

…type_hints # Conflicts: # pglookout/pglookout.py # test/test_pglookout.py

Mulugruntz requested review from alexole and a team April 5, 2023 11:44

kathia-barahona reviewed Apr 20, 2023

View reviewed changes

Mulugruntz requested a review from kathia-barahona April 20, 2023 15:05

alanfranz removed their assignment Apr 28, 2023

Mulugruntz removed the request for review from alanfranz May 2, 2023 14:48

		if "remote_conns" in config:
		config["remote_conns"] = {name: get_connection_info(info) for name, info in config["remote_conns"].items()}

		from typing import Literal, TypedDict


		class Statsd(TypedDict, total=False):

	class MemberState(TypedDict, total=False):
	class ClusterMemberState(TypeDict, total=False):

	assert self.own_db is not None
	assert self.own_db is not None # help mypy

mypy: Add type hints to pglookout [BF-1560] #106

Are you sure you want to change the base?

mypy: Add type hints to pglookout [BF-1560] #106

Conversation

Mulugruntz commented Mar 23, 2023 • edited Loading

Important Note

codecov-commenter commented Mar 27, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanfranz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathia-barahona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanfranz commented Apr 28, 2023

Mulugruntz commented May 1, 2023 • edited Loading

Mulugruntz commented Mar 23, 2023 •

edited

Loading

codecov-commenter commented Mar 27, 2023 •

edited

Loading

alanfranz left a comment •

edited

Loading

Mulugruntz commented May 1, 2023 •

edited

Loading