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
Implement mgmt resource #47944
base: main
Are you sure you want to change the base?
Implement mgmt resource #47944
Changes from 33 commits
f041f24
badf1a5
69fa28a
615fec1
d136be2
a040627
0296a71
fda752a
580508a
ba12225
ef83882
120e164
e4bbde0
b2842c4
5f5cee9
41cf53c
7bba967
5d3a879
2fe508f
d9fd368
17b2a0f
45a10a7
b06fb59
c98e231
848e6d7
0db68cb
2b15650
51bd7f3
a8b2b8f
dc2b83d
adbcc1c
bb92673
af9bd9c
c5ca437
3ed339d
d800d35
8d78f7f
8fbc8aa
081696d
f9e8afd
8d7b831
4dcccc7
d00ee3a
0086b11
e34db7b
417af63
965e951
579dd9b
682e704
22e08c6
541193c
20cdaaf
78b5cb5
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.
Isn't your parent just a property on the client from typespec?
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.
For TypeSpec, we should be able to get the resource hierarchy directly via #48281.
The resource detection logic is mainly for swagger input.
For now, given
getArmResources
API is not ready for integration. We use it as a temporary workaround.Eventually, when we have the native way ready, we should get rid of them. And the resource detection should be part of the M4 output conversion to tspCodeModel.json in autorest.csharp.
Added TODO to remove them.
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.
As offline discussed, we need to clarify the contract of input types containing the resource hierarchy. And it should be an extensible way of holding it in Azure plugin only.
Then, we can put the temp workaround of resource detection close to the input instead of output.
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.
Isolated Resource related logic in ResourceBuilder, so we have separated the temp workaround to a central place.
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 is a lot of code in here that is mgmt specific and it starts to get difficult to tell which is which have we considered a mgmt plugin?
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.
Most likely.
Given I am fully working with MPG now, I think it would be better to split them when we have more implementation on DPG, then we can clarify the things in common for both.
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.
#48481 to track the split, it will happen soon after this PR.