Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

MPG, common model ManagedServiceIdentity defers between typespec-azure and azure-rest-api-specs #2549

Open
XiaofeiCao opened this issue Feb 6, 2024 · 1 comment
Assignees
Labels
Mgmt This issue is related to a management-plane library.

Comments

@XiaofeiCao
Copy link
Contributor

XiaofeiCao commented Feb 6, 2024

Network analytics:
https://github.com/Azure/azure-rest-api-specs/blob/f776434f63fb6505926273db8d4f9a93b75ee4a1/specification/networkanalytics/NetworkAnalytics.Management/main.tsp#L147

@doc("The data product resource.")
@added(Versions.v2023_11_15)
model DataProduct is TrackedResource<DataProductProperties> {
  @doc("The data product resource name")
  @key("dataProductName")
  @segment("dataProducts")
  @path
  @pattern("^[a-z][a-z0-9]*$")
  @minLength(3)
  @maxLength(63)
  name: string;

  ...ManagedServiceIdentity;
}

After spreading, it's a model decorated with @armCommonDefinition:
https://github.com/Azure/typespec-azure/blob/ebfe63960277356c611f15b2404a0ae6f2d9e6ed/packages/typespec-azure-resource-manager/lib/arm.foundations.tsp#L363-L377

@armCommonDefinition(
  "ManagedServiceIdentity",
  {
    version: Azure.ResourceManager.CommonTypes.Versions.v4,
    isDefault: true,
  },
  "managedidentity.json"
)
@armCommonDefinition(
  "ManagedServiceIdentity",
  Azure.ResourceManager.CommonTypes.Versions.v5,
  "managedidentity.json"
)
@doc("The properties of the managed service identities assigned to this resource.")
model ManagedIdentityProperties {
  @doc("The Active Directory tenant id of the principal.")
  @visibility("read")
  tenantId?: string;

  @doc("The active directory identifier of this principal.")
  @visibility("read")
  principalId?: string;

  @doc("The type of managed identity assigned to this resource.")
  type: ManagedIdentityType;

  @doc("The identities assigned to this resource by the user.")
  userAssignedIdentities?: Record<UserAssignedIdentity>;
}

typespec-autorest will generate it to refer to the common model:
https://github.com/Azure/azure-rest-api-specs/blob/f776434f63fb6505926273db8d4f9a93b75ee4a1/specification/networkanalytics/resource-manager/Microsoft.NetworkAnalytics/stable/2023-11-15/networkanalytics.json#L1617

"identity": {
  "$ref": "../../../../../common-types/resource-management/v4/managedidentity.json#/definitions/ManagedServiceIdentity",
  "description": "The managed service identities assigned to this resource."
}

Their name and properties defer:

  1. model name: ManagedServiceIdentity(swagger) vs ManagedIdentityProperties(tsp). ManagedIdentityProperties has @armCommonDefinition, maybe it can be solved by getting the decorator value.
  2. property type:ManagedServiceIdentityType(swagger) vs ManagedIdentityType(tsp) ManagedIdentityType doesn't have @armCommonDefinition on it.. This one's trickier.

Potential solution

  1. We ask typespec-azure-resource-manager to fix the mismatched model property(ManagedServiceIdentityType instead of ManagedIdentityType).
    Reason: Why define the model and its properties if it's properties are wrong?
    Con: Each version(e.g. v4, v5) may have common models different from each other.
  2. We replace the model with the common referenced one(like m4 did).
    Reason: Totally align with current swagger.
    Con: Needs more effort. Do we want to implement it ourselves? Or do we do it in TCGC?
  3. Hard code the property names, make it predefined models in codegen.
    Reason: Easiest way.
    Con: same as 1.
@XiaofeiCao XiaofeiCao self-assigned this Feb 6, 2024
@XiaofeiCao XiaofeiCao added the Mgmt This issue is related to a management-plane library. label Feb 6, 2024
@XiaofeiCao XiaofeiCao changed the title MPG, replace arm-common-type model with common model MPG, common model ManagedServiceIdentity defers between typespec-azure and azure-rest-api-specs Feb 6, 2024
@XiaofeiCao
Copy link
Contributor Author

Current discrepancy: #2766

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

No branches or pull requests

1 participant