Dev appplatform microsoft.app platform 2020 11 01 preview #13336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…AppPlatform-2020-11-01-preview add optional field: ResourceRequests in the swagger
Swagger Validation Report
|
| Rule | Message |
|---|---|
|
The new version has a new property 'resourceRequests' in response that was not found in the old version. New: Microsoft.AppPlatform/preview/2020-11-01-preview/appplatform.json#L3925:7 Old: Microsoft.AppPlatform/preview/2020-11-01-preview/appplatform.json#L3925:7 |
️️✔️LintDiff succeeded [Detail] [Expand]
Validation passes for LintDiff.
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️[Staging] Cross Version BreakingChange (Base on preview version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️[Staging] Cross Version BreakingChange (Base on stable version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️[Staging] SDK Track2 Validation: 3 Warnings warning [Detail]
- The following tags are being changed in this PR
| Rule | Message |
|---|---|
| "readme":"appplatform/resource-manager/readme.md", "tag":"package-preview-2020-11", "details":"The schema 'CloudError' with an undefined type and decalared properties is a bit ambigious. This has been auto-corrected to 'type:object'" |
|
| "readme":"appplatform/resource-manager/readme.md", "tag":"package-preview-2020-11", "details":"The schema 'CloudErrorBody' with an undefined type and decalared properties is a bit ambigious. This has been auto-corrected to 'type:object'" |
|
| "readme":"appplatform/resource-manager/readme.md", "tag":"package-preview-2020-11", "details":"Checking for duplicate schemas, this could take a (long) while. Run with --verbose for more detail." |
️️✔️[Staging] PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️[Staging] SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
Swagger Generation Artifacts
|
|
Hi, @neuqlz Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vsswagger@microsoft.com |
|
Hi, @neuqlz your PR are labelled with WaitForARMFeedback. A notification email will be sent out shortly afterwards to notify ARM review board(armapireview@microsoft.com). cc @raych1 |
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
This was referenced Mar 9, 2021
|
Hi @neuqlz, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review. |
filizt
reviewed
Mar 10, 2021
| "description": "Deployment resource request payload", | ||
| "type": "object", | ||
| "properties": { | ||
| "cpu": { |
There was a problem hiding this comment.
"cpu": { [](start = 6, length = 10)
As the property name this is not clear how the unit should be. I'd recommend making the name a bit clear and changing the type to integer. Please see an example of how both cpu and memory can be defined here:
There was a problem hiding this comment.
It is by design, we want to make cpu and memory field in our API align with the AKS Resource requests convention: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
For cpu, the user can pass 1 or 1000m to represent the 1c.
For memory, the user can pass 1024Mi or 1Gi to represent the 1Gb.
For now, our system needs to support the size like 0.5c/0.5Gb App, the previous cpu and memoryInGb properties are integer type, so they can't represent the 0.5c/0.5Gb size, so we introduce a new field resourceRequests into the API request.
properties: {
cpu: string
memoryInGb: string
resourceRequests: {
cpu: string
memory: string
}
}
The user can use ethier cpu/memory or resourceRequests to represent his App's size.
There was a problem hiding this comment.
I see - thanks for the explanation.
filizt
reviewed
Mar 10, 2021
| @@ -9,8 +9,10 @@ | |||
| "artifactSelector": "sub-module-1" | |||
| }, | |||
| "deploymentSettings": { | |||
| "cpu": 1, | |||
| "memoryInGB": 3, | |||
There was a problem hiding this comment.
Is the removing of these properties intentional? I still see them in the deploymentSettings response model.
There was a problem hiding this comment.
Yep, it is intentional. According to current design, the users can use either cpu/memory or resourceRequests to represent his App's size. It is a straight either or, they don't need to set them all.
|
The PR has breaking changes, please go through the breaking changes review separately from ARM API review. Instructions for breaking change review should be added by the bot above. |
|
As for now, we decide to do the change in a new preview version, so close this PR right now, please refer to this PR: #13256 Sorry for the disturbing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Please ensure to add changelog with this PR by answering the following questions.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.