Skip to content
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

Customization for arm build-in model's visibility and optional setting for resource properties property #131

Open
Tracked by #72
tadelesh opened this issue Jan 16, 2024 · 7 comments · May be fixed by #833
Open
Tracked by #72
Assignees
Milestone

Comments

@tadelesh
Copy link
Member

tadelesh commented Jan 16, 2024

For current ARM resource model the properties property is optional and with visibility read and create, but existed RP's swagger may have different setting for them. Other than redefine a new ProxyResource model, is there other way to do customization for them?
e.g.: https://github.com/Azure/azure-rest-api-specs/blob/05c4049bc22f0ec65acc18f9835132397049cb9e/specification/elasticsan/resource-manager/Microsoft.ElasticSan/stable/2023-01-01/elasticsan.json#L1652

@tadelesh tadelesh changed the title Customization for arm build-in model Customization for arm build-in model's visibility and optional properties Jan 16, 2024
@tadelesh tadelesh changed the title Customization for arm build-in model's visibility and optional properties Customization for arm build-in model's visibility and optional setting Jan 16, 2024
@markcowl markcowl self-assigned this Jan 16, 2024
@markcowl
Copy link
Member

Because the typing of templates is structural and not nominal, you can use your own resource types if you want a different 'properties' optionality/visibility.

The optionality and visibility in the templates is based on current LintDiff rules and required behaviors for RPaaS clients, so there is a disincentive to update the templates.

Need to understand how this impacts management client generation before we think about adding an extra degree of freedom to templates, but because a custom resource type should work just fine, this should not be blocking.

@tadelesh
Copy link
Member Author

tadelesh commented Jan 17, 2024

It will cause breaking change for lots of languages as required property will become a param in default model ctor. Confirm that custom resource type could resolve it. But it seems this is a common issue for lots of existed swaggers. To prevent too much manual work, I'm going to change conversion logic not to use predefined resource template, instead extends predefined resource base and alway add properties property separately.

@markcowl
Copy link
Member

markcowl commented Jan 17, 2024

I think this is related to the inheritance hierarchy for resources as well: #104 - given how many resources don't conform to the represented TrackedResource/ProxyResource/ExtensionResource hierarchy, we may not want to use the existing model templates anyway

@markcowl markcowl removed their assignment Jan 17, 2024
@markcowl markcowl self-assigned this Jan 22, 2024
@markcowl
Copy link
Member

@markcowl ensure that swagger converter has a clear path to avoid breaking client changes

@tadelesh tadelesh removed the Blocking label Jan 25, 2024
@markcowl markcowl removed their assignment Jan 26, 2024
@markcowl markcowl modified the milestone: Backlog Jan 26, 2024
@MaryGao
Copy link
Contributor

MaryGao commented Mar 11, 2024

@tadelesh @markcowl could you help me understand on how this issue is going to resolve?

In our case the impacted properties are id and type in Resource type. In swagger they are 1) all optional and 2) sub types could change to required ones if wanted. In typespec they are all required and no way to change the optionality.

This would cause a big breaking for JS(here) and I was wondering how we could mitigate this.

@tadelesh
Copy link
Member Author

If service team insist on setting visibility and optionality which is different from current arm template, we will use customized definition to define the resource which will make the visibility and optionality correct.

@MaryGao
Copy link
Contributor

MaryGao commented Mar 12, 2024

Thanks and it works for me!

@tadelesh tadelesh changed the title Customization for arm build-in model's visibility and optional setting Customization for arm build-in model's visibility and optional setting for resource properties property Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants