containerapps: fixing inconsistencies in the swagger #18660
Conversation
|
Hi, @tombuildsstuff 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. vscswagger@microsoft.com |
|
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
ghost
added
the
customer-reported
|
Thank you for your contribution tombuildsstuff! We will review the pull request and get back to you soon. |
Swagger Validation Report
|
| Rule | Message |
|---|---|
| Consider using x-ms-client-flatten to provide a better end user experience Location: Microsoft.App/preview/2022-01-01-preview/ManagedEnvironments.json#L792 |
|
| Consider using x-ms-client-flatten to provide a better end user experience Location: Microsoft.App/preview/2022-01-01-preview/ManagedEnvironmentsStorages.json#L290 |
|
| Consider using x-ms-client-flatten to provide a better end user experience Location: Microsoft.App/stable/2022-03-01/ManagedEnvironments.json#L854 |
|
| Consider using x-ms-client-flatten to provide a better end user experience Location: Microsoft.App/stable/2022-03-01/ManagedEnvironmentsStorages.json#L290 |
|
| Based on the response model schema, operation 'ContainerAppsRevisionReplicas_ListReplicas' might be pageable. Consider adding the x-ms-pageable extension. Location: Microsoft.App/preview/2022-01-01-preview/ContainerAppsRevisions.json#L281 |
|
| Based on the response model schema, operation 'ContainerAppsRevisionReplicas_ListReplicas' might be pageable. Consider adding the x-ms-pageable extension. Location: Microsoft.App/stable/2022-03-01/ContainerAppsRevisions.json#L289 |
|
| The child tracked resource, 'daprComponents' with immediate parent 'ManagedEnvironment', must have a list by immediate parent operation. Location: Microsoft.App/preview/2022-01-01-preview/DaprComponents.json#L258 |
|
| The child tracked resource, 'storages' with immediate parent 'ManagedEnvironment', must have a list by immediate parent operation. Location: Microsoft.App/preview/2022-01-01-preview/ManagedEnvironmentsStorages.json#L281 |
|
| The child tracked resource, 'revisions' with immediate parent 'ContainerApp', must have a list by immediate parent operation. Location: Microsoft.App/stable/2022-03-01/ContainerAppsRevisions.json#L393 |
|
| The child tracked resource, 'replicas' with immediate parent 'Revision', must have a list by immediate parent operation. Location: Microsoft.App/stable/2022-03-01/ContainerAppsRevisions.json#L500 |
|
| The child tracked resource, 'daprComponents' with immediate parent 'ManagedEnvironment', must have a list by immediate parent operation. Location: Microsoft.App/stable/2022-03-01/DaprComponents.json#L311 |
|
| The child tracked resource, 'certificates' with immediate parent 'ManagedEnvironment', must have a list by immediate parent operation. Location: Microsoft.App/stable/2022-03-01/ManagedEnvironments.json#L845 |
|
| The child tracked resource, 'storages' with immediate parent 'ManagedEnvironment', must have a list by immediate parent operation. Location: Microsoft.App/stable/2022-03-01/ManagedEnvironmentsStorages.json#L281 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: ready Location: Microsoft.App/preview/2022-01-01-preview/ContainerAppsRevisions.json#L554 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: started Location: Microsoft.App/preview/2022-01-01-preview/ContainerAppsRevisions.json#L558 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: internal Location: Microsoft.App/preview/2022-01-01-preview/ManagedEnvironments.json#L610 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: enabled Location: Microsoft.App/stable/2022-03-01/ContainerApps.json#L558 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: external Location: Microsoft.App/stable/2022-03-01/ContainerApps.json#L594 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: allowInsecure Location: Microsoft.App/stable/2022-03-01/ContainerApps.json#L637 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: latestRevision Location: Microsoft.App/stable/2022-03-01/ContainerApps.json#L710 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: ready Location: Microsoft.App/stable/2022-03-01/ContainerAppsRevisions.json#L562 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: started Location: Microsoft.App/stable/2022-03-01/ContainerAppsRevisions.json#L566 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: internal Location: Microsoft.App/stable/2022-03-01/ManagedEnvironments.json#L667 |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️~[Staging] ApiReadinessCheck succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️Cross-Version Breaking Changes succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️SDK Track2 Validation succeeded [Detail] [Expand]
Validation passes for SDKTrack2Validation
- The following tags are being changed in this PR
️️✔️PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
Swagger Generation Artifacts
|
| "Multiple", | ||
| "Single" |
There was a problem hiding this comment.
in this case, the API returns these in TitleCase:
{
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/stedev-ca-regtest/providers/Microsoft.App/containerApps/stedev-ca-regtest",
"name": "stedev-ca-regtest",
"type": "Microsoft.App/containerApps",
"location": "West Europe",
"systemData": {
"createdBy": "XXX",
"createdByType": "User",
"createdAt": "2022-04-08T10:38:17.4255847",
"lastModifiedBy": "XXX",
"lastModifiedByType": "User",
"lastModifiedAt": "2022-04-08T10:38:17.4255847"
},
"properties": {
"provisioningState": "Succeeded",
"managedEnvironmentId": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/stedev-ca-regtest/providers/Microsoft.App/managedenvironments/stedev-ca-regtest",
"outboundIpAddresses": [
"20.31.240.94",
"20.31.240.109",
"20.31.240.114"
],
"latestRevisionName": "stedev-ca-regtest--bkb6i1h",
"latestRevisionFqdn": "stedev-ca-regtest--bkb6i1h.ashycoast-1ca0508f.westeurope.azurecontainerapps.io",
"customDomainVerificationId": "XXXXXXX",
"configuration": {
"activeRevisionsMode": "Single",
"ingress": {
"fqdn": "stedev-ca-regtest.ashycoast-1ca0508f.westeurope.azurecontainerapps.io",
"external": true,
"targetPort": 80,
"transport": "Auto",
"traffic": [
{
"weight": 100,
"latestRevision": true
}
],
"allowInsecure": false
},
"registries": []
},
"template": {
"containers": [
{
"image": "mcr.microsoft.com/azuredocs/containerapps-helloworld:latest",
"name": "simple-hello-world-container",
"command": [],
"resources": {
"cpu": 0.25,
"memory": "0.5Gi"
}
}
],
"scale": {
"maxReplicas": 10
}
}
},
"identity": {
"type": "None"
}
}
| "multiple", | ||
| "single" | ||
| "Multiple", | ||
| "Single" |
There was a problem hiding this comment.
in this case, the API returns these in TitleCase:
{
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/stedev-ca-regtest/providers/Microsoft.App/containerApps/stedev-ca-regtest",
"name": "stedev-ca-regtest",
"type": "Microsoft.App/containerApps",
"location": "West Europe",
"systemData": {
"createdBy": "XXX",
"createdByType": "User",
"createdAt": "2022-04-08T10:38:17.4255847",
"lastModifiedBy": "XXX",
"lastModifiedByType": "User",
"lastModifiedAt": "2022-04-08T10:38:17.4255847"
},
"properties": {
"provisioningState": "Succeeded",
"managedEnvironmentId": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/stedev-ca-regtest/providers/Microsoft.App/managedenvironments/stedev-ca-regtest",
"outboundIpAddresses": [
"20.31.240.94",
"20.31.240.109",
"20.31.240.114"
],
"latestRevisionName": "stedev-ca-regtest--bkb6i1h",
"latestRevisionFqdn": "stedev-ca-regtest--bkb6i1h.ashycoast-1ca0508f.westeurope.azurecontainerapps.io",
"customDomainVerificationId": "XXXXXXX",
"configuration": {
"activeRevisionsMode": "Single",
"ingress": {
"fqdn": "stedev-ca-regtest.ashycoast-1ca0508f.westeurope.azurecontainerapps.io",
"external": true,
"targetPort": 80,
"transport": "Auto",
"traffic": [
{
"weight": 100,
"latestRevision": true
}
],
"allowInsecure": false
},
"registries": []
},
"template": {
"containers": [
{
"image": "mcr.microsoft.com/azuredocs/containerapps-helloworld:latest",
"name": "simple-hello-world-container",
"command": [],
"resources": {
"cpu": 0.25,
"memory": "0.5Gi"
}
}
],
"scale": {
"maxReplicas": 10
}
}
},
"identity": {
"type": "None"
}
}
|
Hi @tombuildsstuff, Your PR has some issues. Please fix the CI sequentially by following the order of
|
|
Hi @tombuildsstuff, 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. |
tombuildsstuff
force-pushed
the
b/containerapps
branch
from
c7d8448
to
3c4872f
Compare
|
@JeffreyRichter a thought which came to mind when putting this together - have you considered adding a linter to ensure these method parameters are consistent? This seems like something which should be consistent across all usages of a given Resource? |
|
@tombuildsstuff I don't have enough context on this PR. Do you want the linter tool to check casing for consistency or do you want consistent words (multiple/single) to be used across services? I think the former. Azure's guidelines are to use camel casing. I would have thought the linter already checks this. Perhaps @mikekistler can look into this. |
There was a problem hiding this comment.
I do not think you should be making any changes to the 2022-01-01-preview API. Since there is now a stable 2022-03-01 api version, the preview should be retired 90 days after the stable is available, so there is little value in making changes at this point.
As for the changes to 2022-03-01, these will result in breaking changes to SDKs generated from this API definition, so could break customers. We need to avoid that unless absolutely necessary, and "inconsistency" does not meet that bar. Can you give an explanation for why this change is absolutely necessary?
I'm referring to the segments within the URI for user specified segments - if multiple resources are operating on the same URI those segments should be consistently named and cased, for example consider the following endpoints:
Whilst these URIs are all valid in the Swagger, these inconsistencies lead to inconsistencies within the SDKs etc where these are used as method parameters. Instead if these were all consistent:
When the methods are generated for those, users have a consistent parameter name for a given value - which has UX benefits. As such I'm proposing a linter to ensure that where multiple Swagger Resources are operating on the same Resource, that the linter validates that the same parameter name (and casing, as you've mentioned these should be Per this comment/API response - the Swagger doesn't match the API definition, so this isn't correctly defined for both API's either way: #18660 (comment)
We can revert these if needed, that's fine - at this point in time only the Preview API is used in SDK's (AFAIK) - the Stable API doesn't appear to be rolled out either to Azure Public or the SDKs.
Is this a new policy? There's many Azure API's which don't comply with this right now (SQL comes to mind, which due to using a composite API version still makes use of Preview functionality from 2015 iirc).
There's two sets of changes included for each API version here:
In the case of updating the values used for the constant - these are the actual values returned from the API, see this comment for an example response payload: XXX. If needs be we can revert this for now, but given this is what the API is returning (and that AFAIK the Stable API isn't rolled out to SDKs yet) - I don't believe this is a breaking change at this point in time (for the Stable API, at least)? For the other - as mentioned above we're making use of the Resource URI as an Identifier, which gets transformed into an object (in our case a Go struct), which we then pass into SDK methods rather than the individual components. This has the UX benefit of highlighting which resource will be modified by which operation, verses outputting the individual fields - and also reduces the amount of code needed (since you can build up the object one time, then create/update/retrieve/delete it as needed). As it stands right now these URIs being inconsistent means that we're seeing the following perpetual diff:
This means that we're unable to support this resource within Terraform due to the perpetual changes - as such this PR fixes this consistency issue so that we can support this. As mentioned in the first half of this comment, I think there's UX benefits for end-users in having these consistent (since this'll lead to consistent parameter names in the SDKs etc) - so I'm wondering if there's potentially a missing linter for this too? Hope that helps clarify that - but please let us know if there's anything else you need to get this merged. |
|
On the specific topic of the linter rule, you are right that the names should be consistent and the linter can/should check for this. There is already an issue open to add this linter rule to the standard PR checks: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1424461 I added a comment there to hopefully raise the priority on this. And it is already implemented in the Azure API Style Guide linter rules -- here's a screen cap of the linter running in VSCode flagging this particular issue:
So in short, we agree on using consistent names! |
|
On the other points in this PR:
tl;dr Keep the constant name changes and path param changes in the stable version only, come to the Breaking Changes office hours and plead your case that the API is not yet rolled out, and there's a fair chance this will be approved. |
|
@mikekistler , @tombuildsstuff - both 2022-03-01 and 2022-01-01-preview API versions have been rolled out in production, but the SDK clients haven't been released for any of those versions. Because of that I think it should be ok to merge this PR as it now matches the casing to what RP actually returns. Thanks @tombuildsstuff for creating this PR - it is unfortunate we haven't caught the casing of the enum values earlier. |
|
Since this PR is flagged as "BreakingChangeReviewRequired", please open a breaking change intake and come to the next Breaking Change office hours (Mondays 10-11 PT). I don't expect any issues with getting this approved but it should go through the process. |
|
@mikekistler - our team also create this PR to fix a few other case inconsistencies for the enum values. #18839 |
|
@tombuildsstuff - can you please update the PR to be in sync with the latest state of the main branch? |
|
Closing as per this comment since this has been merged via main - thanks both. |
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Add a changelog entry for 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
Otherwise your PR may be subject to ARM review requirements. Complete the following:
Check this box if any of the following apply to the PR so that label "WaitForARMFeedback" will be added automatically to begin ARM API Review. Failure to comply may result in delays to the manifest.
-[ ] To review changes efficiently, ensure you are using OpenAPIHub to initialize the PR for adding a new version. More details, refer to the wiki.
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 any of the following scenarios apply to the PR, request approval from the 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.