Skip to content
This repository was archived by the owner on Jun 16, 2026. It is now read-only.

Request path pattern for mgmt plane SDK#1565

Merged
ArthurMa1978 merged 211 commits into
Azure:feature/v3from
ArcturusZhang:second-step-of-removing-operation-groups
Nov 16, 2021
Merged

Request path pattern for mgmt plane SDK#1565
ArthurMa1978 merged 211 commits into
Azure:feature/v3from
ArcturusZhang:second-step-of-removing-operation-groups

Conversation

@ArcturusZhang

@ArcturusZhang ArcturusZhang commented Oct 13, 2021

Copy link
Copy Markdown
Member

Description

Fixes #1581
Fixes #1609
Fixes #1608
Fixes #1593

This PR includes the following:

  1. Introduce a new class MgmtTypeProvider, which inherits from TypeProvider. This class includes the information that will be generated as a class with operations, like Resource, ResourceContainer and the extension classes.
  2. Now we write methods using a group of operations under the same request path, instead of OperationGroups.
  3. Now we determine the parent-child relationship using request path, instead of relying on OperationGroups or configurations. But this is configurable.
  4. The new request path pattern could identify the singleton resources with an enum with only one value. But due to some updates on autorest.modelerfour, we need to add a flag to enforce the legacy behaviour (see the storage sample).
  5. Introduce the contextual request paths to solve the issue of incorrect parameters during RestOperation invocation.
  6. Support non-stringlike contextual parameters (like int)
  7. Rewrite all the writers to make sure the generated code will have the same pattern.

Known issues and future works:

  1. The item type of pageable methods in extension classes are not wrapped by its corresponding Resource type if it is a resource data. Considering that we might have multiple resources with the same resource data, I do not think this is good approach to do in our SDK which might lead to potential breaking changes.
    But GetAll methods in the container class will still wrap with its corresponding resource.
  2. Configuration to merge operations and resources are not implemented in this PR yet.
  3. Configuration flag to suppress some error into warnings and throw errors by default if the generator cannot decide something
  4. Some ResourceTypes are magic string instead of using the constant from the corresponding classes.
  5. Despite we can encapsulate multiple operations into the same resource or resource container class, we do not really validate them to have the same parameters and return value. This might introduce compile errors in the generated code.

Checklist

To ensure a quick review and merge, please ensure:

  • The PR has a understandable title and description explaining the why and what.
  • The PR is opened in draft if not ready for review yet.
    • If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
  • The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

  • Build is completely green
    • Submissions with test failures require tracking issue and approval of a CODEOWNER
  • At least one +1 review by a CODEOWNER
  • All -1 reviews are confirmed resolved by the reviewer
    • Override/Marking reviews stale must be discussed with CODEOWNERS first

ArcturusZhang and others added 30 commits September 3, 2021 16:47
# Conflicts:
#	src/AutoRest.CSharp/Mgmt/AutoRest/MgmtOutputLibrary.cs
#	src/AutoRest.CSharp/Mgmt/Output/ResourceData.cs
…extual-path

# Conflicts:
#	src/AutoRest.CSharp/Mgmt/AutoRest/MgmtOutputLibrary.cs
#	src/AutoRest.CSharp/Mgmt/Output/ResourceData.cs
@ArcturusZhang ArcturusZhang requested a review from m-nash November 2, 2021 07:23
@ArcturusZhang ArcturusZhang changed the base branch from feature/request-path to feature/v3 November 9, 2021 01:05
@ArcturusZhang ArcturusZhang marked this pull request as ready for review November 9, 2021 03:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

5 participants