-
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
ContainerInstance: Adding April Swagger For GA #2778
Conversation
Automation for azure-libraries-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goNothing to generate for azure-sdk-for-go |
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 add a new tag to specification/containerinstance/resource-manager/readme.md
with the new version
} | ||
} | ||
}, | ||
"Logs": { |
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.
"Logs": { [](start = 1, length = 12)
This type is used only as return value from the server. It is not supposed to be sent back to server thus all the properties should be marked as read-only.
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.
Done
} | ||
}, | ||
"ContainerExecResponse": { | ||
"description": "The information for the container exec command.", |
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.
Same comment here - the type is used only as server response. Please mark all the properties as read-only.
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.
Done
} | ||
}, | ||
"Operation": { | ||
"description": "An operation for Azure Container Instance service.", |
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.
same comment - this is used only as server return value. please mark all the properties as read-only.
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.
Done
"required": [ | ||
"name", | ||
"display" | ||
] |
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.
does not make sense to put required fields in the type that is used only as a server response.
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.
Done
} | ||
}, | ||
"ContainerState": { | ||
"description": "The container instance state.", |
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 name of this type suggest that it is populated only by server - please mark all the properties as read-only
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.
Done
} | ||
} | ||
}, | ||
"Event": { |
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.
Event [](start = 5, length = 5)
same here - user should not modify the events received from server. Please mark properties as read-only.
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.
Done
@ravbhatnagar Could you check if it was approved before? |
@samkreter according to swagger spec specifications https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields-13 |
@hovsepm Just finished the changes you requested. |
de12b25
to
ab364c6
Compare
@samkreter once ARM team will sing off on the new stable version we will be able to merge. |
Signing off from ARM side. |
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger