-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
[Hub Generated] Review request for Microsoft.ContainerRegistry to add version preview/2019-12-01-preview #7883
[Hub Generated] Review request for Microsoft.ContainerRegistry to add version preview/2019-12-01-preview #7883
Conversation
…e/2019-05-01 to version 2019-12-01-preview
Automation for azure-sdk-for-pythonA PR has been created for you: |
Automation for azure-sdk-for-goNothing to generate for azure-sdk-for-go |
These are the global settings for the ContainerRegistry API. | ||
|
||
``` yaml | ||
openapi-type: arm | ||
tag: package-2019-06-preview | ||
tag: package-preview-2019-12 |
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.
How about follow the yyyy-MM-preview
pattern for the tag?
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.
I do not remember changing this file, this file was changed by "OpenApi Hub" on behalf of me.
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.
Thanks for the info. We will log a bug for OpenApi Hub
.
@qiaozha , can you please take a look at the JS SDK failures? |
@jikuma This problem occurs because in your readme.md you've already set your default tag
|
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.
Please see comments
} | ||
} | ||
], | ||
"responses": { |
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 all API responses in this spec, please add a "default" response to capture error cases. It should have a schema consistent with the common error contract. Here is an example:
Line 105 in 0cf7026
"default": { |
Even if this did not exist in prior versions, let's get it added going forward
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.
The swagger file is auto-generated. I do not think we should edit this file. I think we are missing some decorator on the controller for this. Can you point me to an example?
Secondly, I do not want to make changes to the old controller/api as part of this PR. I will open WI to track these changes.
} | ||
} | ||
}, | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ContainerRegistry/registries/{registryName}/listUsages": { |
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 this "listUsages" and not /usages? It should follow the contract.
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.
I do not want to make changes to the old controller/api as part of this PR. I will open WI to track these changes.
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.
Can you help me understand why a new version (a preview too) is not a good opportunity to correct issues? I'm not sure what you mean by "old controller/api"
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.
changes the api and made it usages from listUsages
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.
@KrisBash I see that there is a contract to return the resource usages across Azure. But this API is not exactly the contract implementation as the payload is different and back then we didn't have this contract. Also, it loses its ability to call from ARM template if we remove list prefix. We need to evaluate our API usage and then introduce this change. Can we please take this change as a separate item and scope this version to customer managed key scenarios.
@@ -0,0 +1,1291 @@ | |||
{ |
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 are a number of nested resource types from the prior version not carried forward. Is that intentional? It's a suboptimal experience for the customer to produce a template to configure a resource if they have to use different API versions to configure different element/sub-resources.
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.
Added the deleted controllers back due to consistency.
/openapi sdkautomation |
azure-sdk-for-go - Release
|
azure-sdk-for-java - Release
|
azure-sdk-for-net - Release
|
azure-sdk-for-python - Release
|
azure-sdk-for-js - Release
|
Azure Pipelines successfully started running 1 pipeline(s). |
Can one of the admins verify this patch? |
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.
Discussed offline. Two issues must be fixed in a future update:
- "default" response for all APIs
- listUsages normalization to standard /usages contract
@ArcturusZhang , can you have a look at the SDK Go failures and suggest on the fix? Here's the error:
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
Now that all the checks are passing, can we merge this PR? |
If you are a MSFT employee you can view your work branch via this link.
Contribution checklist: