Issue/6025 implement top level handler abc #6246

Hugo-Inmanta · 2023-07-10T14:48:33Z

Description

Part of #6025

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

Attached issue to pull request
Changelog entry
Type annotations are present
Code is clear and sufficiently documented
No (preventable) type errors (check using make mypy or make mypy-diff)
Sufficient test cases (reproduces the bug/tests the requested feature)
Correct, in line with design
End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

sanderr · 2023-07-10T15:00:46Z

src/inmanta/agent/handler.py

@@ -793,7 +823,7 @@ def call() -> typing.Awaitable[Result]:


 @stable_api
-class CRUDHandler(ResourceHandler):
+class CRUDHandler(ResourceHandler, HandlerABC):


Shouldn't ResourceHandler just inherit from HandlerABC instead?

I guess you mean CRUDHandler ? This would mean we need to lift some functionality from ResourceHandler to HandlerABC ?

No, I mean ResourceHandler. If it inherits from HandlerABC, CRUDHandler doesn't need to. Now we have

class HandlerABC: pass class ResourceHandler: pass class CRUDHandler(ResourceHandler, HandlerABC): pass

and I'm proposing

class HandlerABC: pass class ResourceHandler(HandlerABC): pass class CRUDHandler(ResourceHandler): pass

My first snippet is not right. ~~Not sure if you changed it or~~ I saw it wrong. Anyway, if ResourceHandler(HandlerABC), why would we explicitly inherit from it in CRUDHandler?

Right, 100% agree

sanderr · 2023-07-11T11:32:37Z

src/inmanta/agent/handler.py

@@ -55,7 +55,7 @@ class provider(object):  # noqa: N801
    """
    A decorator that registers a new handler.

-    :param resource_type: The type of the resource this handler provides an implementation for.
+    :param resource_type: The type of the resource this handler provides an implementation for. # TODO rephrase 'provides an implementation for'


Don't forget about this TODO

bartv

Not sure I can contribute here. I a missing missing the point on what the goal of this refactor is.

src/inmanta/agent/handler.py

sanderr · 2023-07-18T08:51:38Z

src/inmanta/agent/handler.py

+        :param resource: The resource to query facts for.
+        """
+
+    @abstractmethod


I don't think this one or pre should be marked as abstractmethod because that requires concrete classes to override them. We want their default behavior to be a NOOP, don't we?

I agree, that's the route I took on my PR implementing the DiscoveryHandler, I'll change it here as well

src/inmanta/agent/handler.py

arnaudsjs · 2023-07-20T05:30:12Z

src/inmanta/agent/handler.py

+    """
+
+    def __init__(self, agent: "inmanta.agent.agent.AgentInstance", io: Optional["IOBase"] = None) -> None:
+        self._agent = agent


Shouldn't we call super here instead of setting self._agent, self._ioloop and self._client directly?

arnaudsjs · 2023-07-20T05:40:55Z

src/inmanta/agent/handler.py

+        requires: Dict[ResourceIdStr, ResourceState],
+    ) -> None:
+        """
+        Main entrypoint of the handler that will be called by the agent.


This docstring doesn't really describe the behavior of the method.

sanderr · 2023-07-20T10:21:49Z

src/inmanta/agent/handler.py

+@stable_api
+class ResourceHandler(HandlerABC):
+    """
+    A baseclass for classes that handle resources. New handler are registered with the


The part about handler registration seems relevant for the top level ABC as well.

sanderr · 2023-07-20T10:23:44Z

src/inmanta/agent/handler.py

+        self,
+        ctx: HandlerContext,
+        resource: R,
+        requires: Dict[ResourceIdStr, ResourceState],


I think this would be a good time to clean up this type. Either dict or abc.Mapping. Afaik we don't ever update it so Mapping would be best. @arnaudsjs do you know?

I don't think we ever update it.

sanderr · 2023-07-20T10:24:46Z

src/inmanta/agent/handler.py

        :return: A client that is associated with the session of the agent that executes this handler.
        """
        if self._client is None:
            self._client = protocol.SessionClient("agent", self._agent.sessionid)
        return self._client

+
+@stable_api
+class ResourceHandler(HandlerABC):


Suggested change

class ResourceHandler(HandlerABC):

class ResourceHandler(HandlerABC[R]):

I think. Then update all type annotations of resources.Resource to R instead. Same for the CRUDHandler.

Co-authored-by: arnaudsjs <[email protected]>

…t-top-level-handler-ABC' into issue/6025-implement-top-level-handler-ABC

Co-authored-by: Sander Van Balen <[email protected]>

arnaudsjs · 2023-08-16T12:06:03Z

Regarding the typing issue on chain_future: This seems to be a typing issue in Tornado. The chain_future is annotated to accept a asyncio.Future instance, but according to the implementation and the documentation of the method it accepts all types of Futures (asyncio.Future and concurrent.futures.Future). I created a ticket for this.

arnaudsjs · 2023-08-16T12:34:36Z

@sanderr I still have the following type error left:

src/inmanta/resources.py:435: error: Incompatible return value type (got "Resource", expected "T")  [return-value]

I don't really see how that can be fixed. Resource.deserialize() is a class method. Its return type depends on what resource.get_class() returns and that can be any subclass of Resource.

inmantaci · 2023-08-17T11:00:42Z

Processing this pull request

inmantaci · 2023-08-17T11:00:44Z

Pull request rejected by merge tool. This pull request has not been approved yet.

Sander is out of office for several says

inmantaci · 2023-08-17T11:04:42Z

Processing this pull request

inmantaci · 2023-08-17T11:04:47Z

Merged into branches master in 4ffeafa

…rce handlers generic in their resource type. (Issue #6025, PR #6246) # Description Part of #6025 # Self Check: Strike through any lines that are not applicable (`~~line~~`) then check the box - [ ] Attached issue to pull request - [ ] Changelog entry - [ ] Type annotations are present - [ ] Code is clear and sufficiently documented - [ ] No (preventable) type errors (check using make mypy or make mypy-diff) - [ ] Sufficient test cases (reproduces the bug/tests the requested feature) - [ ] Correct, in line with design - [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: ) - [ ] If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)

Hugo-Inmanta added 5 commits July 7, 2023 08:35

[WIP]

b55d152

[FIX] fix broken anchor in link

cad6ced

[WIP]

31dec93

[WIP] Implement top-level handler ABC

8dba6f1

[FIX] remove unrelated change

769ab33

Hugo-Inmanta requested a review from sanderr July 10, 2023 14:52

sanderr requested changes Jul 11, 2023

View reviewed changes

sanderr requested a review from bartv July 11, 2023 11:33

[WIP]

15856ba

bartv reviewed Jul 11, 2023

View reviewed changes

[FIX]

61a75cb

arnaudsjs reviewed Jul 17, 2023

View reviewed changes

src/inmanta/agent/handler.py Outdated Show resolved Hide resolved

sanderr reviewed Jul 18, 2023

View reviewed changes

Hugo-Inmanta added 7 commits July 19, 2023 11:21

Merge branch 'master' into issue/6025-implement-top-level-handler-ABC

982db23

[FORMAT]

d1b54b0

[DOC] add changelog

c6d5a69

[WIP]

c4274b8

[FORMAT]

62f9d3c

[WIP]

d7c7996

[WIP]

b1dcdf9

Hugo-Inmanta requested review from sanderr and arnaudsjs July 19, 2023 14:07

arnaudsjs requested changes Jul 20, 2023

View reviewed changes

sanderr requested changes Jul 20, 2023

View reviewed changes

Hugo-Inmanta and others added 5 commits July 21, 2023 12:05

Update src/inmanta/agent/handler.py

66d5008

Co-authored-by: arnaudsjs <[email protected]>

[WIP]

ce2330a

Merge remote-tracking branch 'refs/remotes/origin/issue/6025-implemen…

5312f01

…t-top-level-handler-ABC' into issue/6025-implement-top-level-handler-ABC

[WIP]

a14cdfd

[WIP]

01023e6

Hugo-Inmanta requested a review from wouterdb July 31, 2023 07:03

arnaudsjs and others added 9 commits August 16, 2023 11:13

Update src/inmanta/agent/handler.py

b6bf5a9

Co-authored-by: Sander Van Balen <[email protected]>

Update src/inmanta/agent/handler.py

2c11855

Co-authored-by: Sander Van Balen <[email protected]>

Update src/inmanta/agent/handler.py

50623c3

Co-authored-by: Sander Van Balen <[email protected]>

Update src/inmanta/agent/handler.py

defcafb

Co-authored-by: Sander Van Balen <[email protected]>

Update src/inmanta/agent/handler.py

4157805

Co-authored-by: Sander Van Balen <[email protected]>

Update src/inmanta/agent/handler.py

ce6ca72

Co-authored-by: Sander Van Balen <[email protected]>

Improve changelog entry

d730a1b

Fix typing issues

bab6f27

Merge branch 'master' into issue/6025-implement-top-level-handler-ABC

3ecd993

Fix type annotation in clone() method

7bd99e9

arnaudsjs requested review from sanderr and removed request for sanderr August 16, 2023 12:36

arnaudsjs added the merge-tool-ready This ticket is ready to be merged in label Aug 17, 2023

inmantaci removed the merge-tool-ready This ticket is ready to be merged in label Aug 17, 2023

inmantaci assigned Hugo-Inmanta Aug 17, 2023

arnaudsjs requested review from sanderr and removed request for sanderr August 17, 2023 11:01

arnaudsjs added the merge-tool-ready This ticket is ready to be merged in label Aug 17, 2023

inmantaci closed this Aug 17, 2023

inmantaci deleted the issue/6025-implement-top-level-handler-ABC branch August 17, 2023 11:04

wouterdb mentioned this pull request Aug 21, 2023

Issue/6025 implement discovery handler (2) #6415

Closed

11 tasks

arnaudsjs mentioned this pull request Aug 21, 2023

Improve typing regarding handlers #6416

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/6025 implement top level handler abc #6246

Issue/6025 implement top level handler abc #6246

Hugo-Inmanta commented Jul 10, 2023

sanderr Jul 10, 2023

Hugo-Inmanta Jul 11, 2023

sanderr Jul 11, 2023

sanderr Jul 11, 2023 •

edited

Loading

Hugo-Inmanta Jul 11, 2023

sanderr Jul 11, 2023

bartv left a comment

sanderr Jul 18, 2023

Hugo-Inmanta Jul 18, 2023

arnaudsjs Jul 20, 2023

arnaudsjs Jul 20, 2023

sanderr Jul 20, 2023

sanderr Jul 20, 2023

arnaudsjs Jul 20, 2023

sanderr Jul 20, 2023

arnaudsjs commented Aug 16, 2023

arnaudsjs commented Aug 16, 2023

inmantaci commented Aug 17, 2023

inmantaci commented Aug 17, 2023

inmantaci commented Aug 17, 2023

inmantaci commented Aug 17, 2023

	class ResourceHandler(HandlerABC):
	class ResourceHandler(HandlerABC[R]):

Issue/6025 implement top level handler abc #6246

Issue/6025 implement top level handler abc #6246

Conversation

Hugo-Inmanta commented Jul 10, 2023

Description

Self Check:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartv 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

arnaudsjs commented Aug 16, 2023

arnaudsjs commented Aug 16, 2023

inmantaci commented Aug 17, 2023

inmantaci commented Aug 17, 2023

inmantaci commented Aug 17, 2023

inmantaci commented Aug 17, 2023

sanderr Jul 11, 2023 •

edited

Loading