Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Issue/6025 implement discovery handler #6264
Issue/6025 implement discovery handler #6264
Changes from 21 commits
b55d152
cad6ced
31dec93
8dba6f1
769ab33
5a6919f
15856ba
61a75cb
1b0b7c9
cfc0ce3
9e010ca
247b5e9
7f02a3c
1357fef
69f5439
6996342
9b75118
8f0092d
1f010bb
af7fbdd
4549578
5bcc5e4
9819c83
466dfda
e6c65cd
982db23
d1b54b0
c6d5a69
172fc07
be3ea06
1a70961
c4274b8
62f9d3c
d7c7996
b1dcdf9
3a22bc6
2b67264
b80dafa
3fd115d
66d5008
ce2330a
5312f01
a14cdfd
cd84b14
4db9db7
705024c
806acd1
01023e6
5c62a8a
e863677
cf516ae
ed99bd0
3357e12
8e894c2
8127220
806e730
2f3555b
f3473eb
d99ceee
72a61bb
4c97da5
90f7509
221a0f0
f1f37ed
1fa818d
be04e9a
334fb7f
ebb3580
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this is still missing. The main resource entry-point (at the moment, depends very slightly on the final outcome of the other PR) is
deploy
so that method should be implemented in this class to calldiscover_resources
andreport_discovered_resources
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch that this should be a
ResourceIdStr
, looks like that was missing from the design.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure of whether this logic should happen in a synchronous or asynchronous context.
The testcase currently uses the async version of this (
async_report_discovered_resources
) and passes.I was having second thoughts about it and tried the synchronous version (
report_discovered_resources
) and it looks like it is hanging somewhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All agent ABC methods are synchronous so this should be synchronous as well. I would even go as far as to drop the async method. For testing purposes, using the sync client is more complicated because if both server and client run on the same thread, a synchronous call would hang the server so it can never respond. I'm pretty sure we have some fixtures to work with that, I'll add a suggestion to your test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
deploy
. Could optionally be split into discover -> serialize -> report, so deploy just has to chain them together. No strong preference on the last part, unless the serialization turns out to be very verbose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a style preference but I would always pass
discovered_resources
as a parameter rather than counting on an implicit closure of something that's defined below it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be typed as lowercase
list
orabc.Sequence
. You're not modifying it soabc.Sequence
presents the clearer picture.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
purely a matter of taste, but I would put the
self.discover_resources(ctx, resource)
on a line of its own, as it is the main thing here. It is kind of hidden now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URT
is just aTypeVar
, not the bound type. It is also just an intermediate representation type so I'm not sure if it is even relevant to the end user. Same for the other messageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
repr()
does pretty much this:Same for other message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah turns out it is not the same regarding formatting:
And this test fails if we change it to repr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. To be honest I personally prefer the
repr
output but if we were already using the other approach (I hadn't realized when I commented) I guess we should stick with that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These types do not match, we should serialize here.