2021/11/18 Add AirQuality, Historical, and Tropical API #16802
Conversation
|
Hi, @john35452 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 |
|
[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. |
Swagger Validation Report
|
Swagger Generation Artifacts
|
|
NewApiVersionRequired reason: |
| "RequiredWeatherBasinId": { | ||
| "name": "basinId", |
There was a problem hiding this comment.
You should be able to omit required here and set it elsewhere so you don't have essentially two nearly identical definitions that create a maintenance burden.
There was a problem hiding this comment.
I totally am agree with you, and I tried with this for several time.
But I keep running in to below problems.
An error happens when I add the line to set the parameter to required.
Do you have any suggestion for this?
| }, | ||
| "x-ms-parameter-location": "method" | ||
| }, | ||
| "RequiredWeatherGovernmentId": { |
There was a problem hiding this comment.
Same comment here. You should be able to omit required here and set it elsewhere so you don't have essentially two nearly identical definitions that create a maintenance burden.
|
Hi @john35452, Your PR has some issues. Please fix the CI sequentially by following the order of
|
be07290 to
ace0d10
Compare
|
@tjprescott |
|
I created a Python APIView of the diff to better comment on the resulting API: |
There was a problem hiding this comment.
Several more comments direclty in the accompanying APIView:
https://apiview.dev/Assemblies/Conversation/5664808ac3ec49f3ab287f3c9e85f55b
|
@john35452 I completed a full review of the changes via APIView. There are comments in this review but many more comments in the APIView. Feel free to reach out if you have questions/concerns. |
|
@tjprescott Really appreciate for all the comments. I have updated my code. Please help me to review them again. |
92dfe39 to
0beb62b
Compare
custom-words.txt
Outdated
| servermetrics | ||
| Obuscated |
There was a problem hiding this comment.
I don't believe "Obuscated" is a word. You probably want "Obfuscated".
There was a problem hiding this comment.
Good catch!
I also notice this is not used any more, so I remove this term from the custom-words list.
| "type": "string", | ||
| "in": "query", | ||
| "required": true, | ||
| "default": "2.0", |
There was a problem hiding this comment.
A required parameter should not specify a default value.
There was a problem hiding this comment.
Thanks a lot! I have removed the default value here.
| "type": "integer" | ||
| }, | ||
| { | ||
| "$ref": "../../../Common/preview/1.0/common.json#/parameters/Language" |
There was a problem hiding this comment.
If this is a "stable" (i.e. GA) API version, shouldn't you be using a stable version of this common type?
There was a problem hiding this comment.
Yes. Ideally it should be like that.
However, currently there is no stable version of common library and this common library is used in other stable api documentation as well.
https://github.com/Azure/azure-rest-api-specs/tree/main/specification/maps/data-plane/Common
There was a problem hiding this comment.
There are at least two ways to deal with this:
- Copy the schemas you need from the Common file into your API, or
- Create a "stable" version of the Common file.
Leaving it as is is highly not recommended.
There was a problem hiding this comment.
After consulting SDK members, they will move the related files to stable version once the SDKs are launched.
| "name": "duration", | ||
| "description": "Specifies for how many days the daily forecast responses are returned. Available values are\n * `1` - Return forecast data for the next day. Returned by default.\n * `5` - Return forecast data for the next 5 days.\n * `10` - Return forecast data for the next 10 days.\n * `25` - Return forecast data for the next 25 days. Only available in S1 SKU.\n * `45` - Return forecast data for the next 45 days. Only available in S1 SKU.", | ||
| "in": "query", | ||
| "type": "integer" |
There was a problem hiding this comment.
Thanks for the suggestion!
This is not the new weather api which is out of scope of this time.
| "name": "details", | ||
| "description": "Return full details for the severe weather alerts. Available values are\n * `true` - Returns full details. By default all details are returned.\n * `false` - Returns a truncated version of the alerts data, which excludes the area-specific full description of alert details (`alertDetails`).", | ||
| "in": "query", | ||
| "type": "string" |
There was a problem hiding this comment.
Here too -- this should be "type: boolean".
There was a problem hiding this comment.
Thanks for the suggestion!
This is not the new weather api which is out of scope of this time.
| "name": "duration", | ||
| "description": "Specifies for how many days the daily indices are returned. By default, the indices data for the current day will be returned. When requesting future indices data, the current day is included in the response as day 1. Available values are\n * `1` - Return daily index data for the current day. Default value.\n * `5` - Return 5 days of daily index data starting from the current day.\n * `10` - Return 10 days of daily index data starting from the current day.\n * `15` - Return 15 days of daily index data starting from the current day.", | ||
| "in": "query", | ||
| "type": "integer" |
There was a problem hiding this comment.
Thanks for the suggestion!
This is not the new weather api which is out of scope of this time.
| } | ||
| }, | ||
| "WeatherWaypoint": { | ||
| "x-ms-client-name": "WaypointForecast", |
There was a problem hiding this comment.
Recommend just changing the schema name rather than using x-ms-client-name here.
There was a problem hiding this comment.
Thanks for the suggestion!
This is not the new weather api which is out of scope of this time.
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "parameters": { | |||
| "api-version": "2.0", | |||
| "api-version": "1.1", | |||
There was a problem hiding this comment.
I thought the discussion at the meeting was for date-based versions since that's what other services with an API version query parameter do?
There was a problem hiding this comment.
Per the discussion and conclusion in the 2nd weather api review session, we would apply api version 1.1 and I've shared the related recording with Travis.
|
Hi @john35452, Your PR has some issues. Please fix the CI sequentially by following the order of
|
|
@markwietzel have the comments from #16993 been addressed? If so, could someone from the API stewardship board update the label from APIStewardship-ChangesRequested to APIStewardship-Approved? |
Hi @markweitzel , Thanks for noting the conclusion of discussion here. API path pattern: Severe alert: API-version: Really appreciate for your review comment! |
* 2021/08/05 Add Tropical api * 2021/11/18 Add air quality api * 2021/11/18 Add Historical api * 2021/11/22 Update enum and add x-ms-client-name * 2021/11/23 Update examples * 2021/11/23 Update naming * 2021/11/23 Create parameter for Position * 2021/11/23 Update active storm * 2021/11/30 Change reponse to result * 2021/11/30 Add x-ms-client-name * 2021/11/30 Update timestamp, coordinate, and radius naming in SDK * 2021/11/30 Use WeatherUnit for Concentration * 2021/11/30 Update Daily Air quality dominantPollutant * 2021/11/30 Unify response format * 2021/11/30 Unify CurrentAirQuality and HourlyAirQualityForecast * 2021/12/01 Update window and weatherValue SDK name * 2021/12/02 Update coordinate and left, right SDK name * 2021/12/03 Add SDK name for results * 2021/12/03 Improve Object name * 2021/12/03 Update ActiveStorm * 2021/12/03 Remove needless object and update name * 2021/12/06 Add weather 2.0 directory * 2021/12/06 Recover Weather 1.0 Swagger file * 2021/12/07 Remove v2 from operationId * 2021/12/08 Update readme.md * Set default API version as 2.0 * 2022/01/07 Remove default value for api-version * 2022/01/07 Remove needless custom-words * 2022/01/11 Update api-version and add nextLink * 2022/01/11 Update api-version in readme.md Co-authored-by: John Lai <tingchiehlai@microsoft.com> Co-authored-by: evaou <speeder528@gmail.com>
* 2021/08/05 Add Tropical api * 2021/11/18 Add air quality api * 2021/11/18 Add Historical api * 2021/11/22 Update enum and add x-ms-client-name * 2021/11/23 Update examples * 2021/11/23 Update naming * 2021/11/23 Create parameter for Position * 2021/11/23 Update active storm * 2021/11/30 Change reponse to result * 2021/11/30 Add x-ms-client-name * 2021/11/30 Update timestamp, coordinate, and radius naming in SDK * 2021/11/30 Use WeatherUnit for Concentration * 2021/11/30 Update Daily Air quality dominantPollutant * 2021/11/30 Unify response format * 2021/11/30 Unify CurrentAirQuality and HourlyAirQualityForecast * 2021/12/01 Update window and weatherValue SDK name * 2021/12/02 Update coordinate and left, right SDK name * 2021/12/03 Add SDK name for results * 2021/12/03 Improve Object name * 2021/12/03 Update ActiveStorm * 2021/12/03 Remove needless object and update name * 2021/12/06 Add weather 2.0 directory * 2021/12/06 Recover Weather 1.0 Swagger file * 2021/12/07 Remove v2 from operationId * 2021/12/08 Update readme.md * Set default API version as 2.0 * 2022/01/07 Remove default value for api-version * 2022/01/07 Remove needless custom-words * 2022/01/11 Update api-version and add nextLink * 2022/01/11 Update api-version in readme.md Co-authored-by: John Lai <tingchiehlai@microsoft.com> Co-authored-by: evaou <speeder528@gmail.com>
This PR is the combination of following PRs.
AirQuality:
#16352
Historical:
#16486
Tropical:
#15532
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 copy the existing version into the new directory structure for first commit and then push new changes, including version updates, in separate commits.
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.