-
Notifications
You must be signed in to change notification settings - Fork 517
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
Onboard msi for autogeneration #764
Conversation
anthony-c-martin
commented
Oct 15, 2019
} | ||
}, | ||
"required": [ | ||
"name", | ||
"type", | ||
"apiVersion", | ||
"location" |
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.
Why is location being removed from the required properties?
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.
It's not been marked as required in your swagger definition:
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/msi/resource-manager/Microsoft.ManagedIdentity/stable/2018-11-30/ManagedIdentity.json#L333-L336
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.
Seems like this "location" is still being removed. However, we marked it as required here:
https://github.com/Azure/azure-rest-api-specs/blob/21460cb6be715578c300a130b9af7acbf277fbc8/specification/msi/resource-manager/Microsoft.ManagedIdentity/stable/2018-11-30/ManagedIdentity.json#L402
How else can we mark it as required?
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.
If I move the "required" field from the Identity definition to the Resource definition, the schema is correctly generated with location as required.
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.
Ok, we have a PR that should address that, using Azure common types: Azure/azure-rest-api-specs#8352
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.
@anthony-c-martin can you try to regenerate? We've updated our swagger to use Azure common types definitions.
Thanks for the info Anthony. We are looking at making the needed updates to our swagger now. After that we can regenerate the changes in this PR (before merging). |