Issue/6025 implement discovery handler (2) #6415

arnaudsjs · 2023-08-21T06:14:06Z

Description

implement DiscoveryHandler
Verify that the Id constructor and resource_str method are part of the stable API and documented.

This PR replaces #6264

Part of #6025

Self Check:

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 (Issue/6025 document discovery handler #6270)
~~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)~~

wouterdb · 2023-08-21T08:27:28Z

src/inmanta/agent/handler.py

@@ -529,15 +518,53 @@ def filter_resources_in_unexpected_state(
            )

    @abstractmethod
+    def _do_execute(self, ctx: HandlerContext, resource: TResource, dry_run: bool = False) -> None:


This is getting very very complicated.

We have 'deploy', 'execute' and now _do_execute and post_execute.

This class was supposed to be a fairly clean interface between agent and handler. I was reluctant against adding an implementation to the deploy method at all (#6246 (comment)) but I thinks this takes it too far.

I think, with this structure, there is so much inversion of control, that to know what each sub class does, you have to read up-and-down the inheritance hierarchy and piece it back together. These methods have no clean function of their own, only in relation to the implementation of other methods.

I see why you lifted out this error handling code, but I wonder if there is a better way.

For post-execute, I think the easiest way of removing this is to have the subclass override execute and do

def execute(...): super().execute(..._ if _should_reload(): self.do_reload(ctx, resource)

For the exception handling, I wonder if we can lift it up/duplicate it higher up in the call stack, that would both make it more robust and more out-of-sight?

I mostly agree with this. A few additional notes:

I think we could consider keeping _do_execute iff we more clearly distinguish the implementor interface from the user interface, keeping the footprint for the handler developer small and intuitive.

Even though I think we can get away with it, I don't necessairly think we should. Is there a reason we can't lift the error handling and the call to pre and post into deploy directly? This may be what Wouter is proposing as well, I'm not sure.

For the post execute I wholeheartedly agree.

Ah, 2 and 3 are not compatible...

I think we could consider keeping _do_execute iff we more clearly distinguish the implementor interface.

I tried to make it clear with the underscore that this was not part of the public API.

Is there a reason we can't lift the error handling and the call to pre and post into deploy directly? This may be what Wouter is proposing as well, I'm not sure.

Wouter was proposing to move it out of the hander. That means moving it into the ResourceAction for example.

The main question is: Do you feel the same way as Wouter? Namely that this implementation results in very complicates and hard to understand code. And if so, do you see a good way to keep the benefits while making it easier to understand? If we cannot come to a better implementation, I would just roll it back and then we just trade better readability for code duplication.

that this implementation results in very complicates and hard to understand code

Code not so much, but I do find that it "pollutes" the interface. Because, while you say it's not part of the public API, handlers are still supposed to implement it. On the other hand I'm inclined to agree that the error handling code belongs in the default implementation because it is part of the generic concept of a resource + handler.

Wouter was proposing to move it out of the hander.

Ok, I misinterpreted that. But my proposal still stands, why not move it to deploy? Afaik that shouldn't break any of our current handlers, and I feel like it would fit with the currently defined contracts of both deploy and execute. Perhaps we have to add one more sentence to the deploy docstring to be explicit but the purpose of deploy is essentially to inspect dependencies' state, trigger the actual deploy (by calling into execute) and react appropriately to any unexpected state. I don't think this would break any of our existing handlers, would it?

I'm leaving post_execute out of it for now because I feel like _do_execute is the big one and the other discussion hinges slightly on which way we go here.

I am fine with moving the error handling logic into deploy(). However, this would mean we also have to move the pre() and post() methods to deploy(). So the behavior will change when someone overrides the execute() method directly. I can't find a module that does that. If we don't have one, I think we can safely move the error handling logic into deploy().

Afaik we don't have one, and this is iso7 so I think we have some freedom here as long as we mention it in a change entry. From where I'm standing, I like this idea best among our options. What about you?

I can't find one either.

This reverts commit fe76a87.

…logic"" This reverts commit a7282af.

sanderr

Very minor change request, otherwise as discussed with regards to the error handling etc.

src/inmanta/agent/handler.py

sanderr · 2023-08-23T08:18:21Z

src/inmanta/agent/handler.py

+
+
+@stable_api
+class DiscoveryHandler(HandlerAPI[TDiscovery], Generic[TDiscovery, TDiscovered]):


Wow, I had no clue it was required to list all type vars here. I really can't wait for the 3.12 syntax.

changelogs/unreleased/6025-implement-discovery-resource-handler.yml

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

…r.yml Co-authored-by: Sander Van Balen <[email protected]>

…om:inmanta/inmanta-core into issue/6025-implement-discovery-handler-bis

changelogs/unreleased/6025-implement-discovery-resource-handler.yml

…r.yml Co-authored-by: Sander Van Balen <[email protected]>

wouterdb

let me know when it is ready again

inmantaci · 2023-08-28T12:09:48Z

Processing this pull request

inmantaci · 2023-08-28T12:09:52Z

Merged into branches master in 80cb4b4

…#6025, PR #6415) # Description - [x] implement `DiscoveryHandler` - [x] Verify that the `Id` constructor and `resource_str` method are part of the stable API and documented. This PR replaces #6264 Part of #6025 # Self Check: - [x] Attached issue to pull request - [ ] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] Correct, in line with design - [x] End user documentation is included or an issue is created for end-user documentation (#6270) - [ ] ~~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)~~

arnaudsjs added 10 commits August 17, 2023 13:43

Small fixes

cb1198e

Add tests

f797916

Enhance tests

a1011e6

Fix tests

c19ae69

Factor out common setup, teardown and error handling logic

fe76a87

Fix typing

540f6b8

Small enhancements

c9bbe75

Fix formatting

50e1df1

Merge branch 'master' into issue/6025-implement-discovery-handler-bis

634f304

Add changelog entry

1cf9641

arnaudsjs requested review from sanderr and wouterdb August 21, 2023 06:18

wouterdb requested changes Aug 21, 2023

View reviewed changes

arnaudsjs added 2 commits August 23, 2023 10:37

Revert "Factor out common setup, teardown and error handling logic"

a7282af

This reverts commit fe76a87.

Revert "Revert "Factor out common setup, teardown and error handling …

44281e6

…logic"" This reverts commit a7282af.

sanderr requested changes Aug 23, 2023

View reviewed changes

arnaudsjs and others added 8 commits August 23, 2023 11:31

Update src/inmanta/agent/handler.py

be4868c

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

Update src/inmanta/agent/handler.py

caf9ab1

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

Update changelogs/unreleased/6025-implement-discovery-resource-handle…

28ba0a2

…r.yml Co-authored-by: Sander Van Balen <[email protected]>

Implement review comments

2846025

Merge branch 'issue/6025-implement-discovery-handler-bis' of github.c…

322efbc

…om:inmanta/inmanta-core into issue/6025-implement-discovery-handler-bis

Merge branch 'master' into issue/6025-implement-discovery-handler-bis

57feffb

Fix changelog message

ab2c3a9

Fix typing

2266406

arnaudsjs requested review from wouterdb and sanderr August 23, 2023 11:24

sanderr requested changes Aug 23, 2023

View reviewed changes

changelogs/unreleased/6025-implement-discovery-resource-handler.yml Outdated Show resolved Hide resolved

Update changelogs/unreleased/6025-implement-discovery-resource-handle…

30e8a3e

…r.yml Co-authored-by: Sander Van Balen <[email protected]>

arnaudsjs requested a review from sanderr August 23, 2023 12:07

sanderr approved these changes Aug 23, 2023

View reviewed changes

wouterdb reviewed Aug 25, 2023

View reviewed changes

arnaudsjs added 4 commits August 25, 2023 14:47

Revert move error handling logic

f7ab0a8

Merge branch 'master' into issue/6025-implement-discovery-handler-bis

8426f89

Implement review comments

1959d44

Small fix

80065dd

arnaudsjs requested a review from wouterdb August 28, 2023 10:21

wouterdb approved these changes Aug 28, 2023

View reviewed changes

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

inmantaci closed this Aug 28, 2023

inmantaci deleted the issue/6025-implement-discovery-handler-bis branch August 28, 2023 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/6025 implement discovery handler (2) #6415

Issue/6025 implement discovery handler (2) #6415

arnaudsjs commented Aug 21, 2023 •

edited

Loading

wouterdb Aug 21, 2023

sanderr Aug 21, 2023

sanderr Aug 21, 2023

arnaudsjs Aug 21, 2023

arnaudsjs Aug 21, 2023 •

edited

Loading

sanderr Aug 21, 2023

arnaudsjs Aug 22, 2023

sanderr Aug 22, 2023

arnaudsjs Aug 22, 2023

sanderr left a comment

sanderr Aug 23, 2023

wouterdb left a comment

inmantaci commented Aug 28, 2023

inmantaci commented Aug 28, 2023



		@stable_api
		class DiscoveryHandler(HandlerAPI[TDiscovery], Generic[TDiscovery, TDiscovered]):

Issue/6025 implement discovery handler (2) #6415

Issue/6025 implement discovery handler (2) #6415

Conversation

arnaudsjs commented Aug 21, 2023 • edited Loading

Description

Self Check:

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 Aug 21, 2023 • 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

Choose a reason for hiding this comment

sanderr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterdb left a comment

Choose a reason for hiding this comment

inmantaci commented Aug 28, 2023

inmantaci commented Aug 28, 2023

arnaudsjs commented Aug 21, 2023 •

edited

Loading

arnaudsjs Aug 21, 2023 •

edited

Loading