Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Users/tolee/workbooks s360 compliance #11746

Merged
merged 42 commits into from
Jan 12, 2021
Merged

Users/tolee/workbooks s360 compliance #11746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
6d7fc10
add valid delete response code
tonykslee Nov 18, 2020
6ed2fe6
update to allow null serialized data
tonykslee Nov 18, 2020
28bc4a2
updated nullable syntax and removed changes from wrong file
tonykslee Nov 19, 2020
84182aa
added more examples for validation
tonykslee Nov 19, 2020
7a85cab
updated myWorkbooks delete response code
tonykslee Nov 19, 2020
029e5ee
commented suppressions to get the errors
tonykslee Nov 19, 2020
f908e21
updated patch response code to 201
tonykslee Nov 19, 2020
da943d3
updated kind details
tonykslee Nov 19, 2020
52f09dc
nullable to storageUri
tonykslee Nov 19, 2020
da280c8
nullable to storageUri
tonykslee Nov 19, 2020
865c9d2
updated nextLink
tonykslee Nov 19, 2020
a418bf8
removed required modifier on location field
tonykslee Nov 19, 2020
89fb6f4
updated error response schema
tonykslee Nov 19, 2020
8db80e4
added more examples and fixed responses
tonykslee Nov 19, 2020
b7f1235
testing out removing non standard resource fields
tonykslee Nov 20, 2020
6c3dee3
added back in existing properties
tonykslee Nov 20, 2020
3fefdbd
removed the failing examples
tonykslee Nov 20, 2020
72cc9f9
added back in the suppressions
tonykslee Nov 20, 2020
6b38d9f
removed unused example
tonykslee Nov 20, 2020
ba2bfe5
removed reference to get2 and add2 example files
tonykslee Nov 20, 2020
a970943
updated workbook_update to only produce a 201 response
tonykslee Nov 20, 2020
b736fdc
updated managedUpdate response code
tonykslee Nov 20, 2020
6ea471e
updated workbook error name
tonykslee Nov 20, 2020
7afa423
added nextLink to examples
tonykslee Nov 20, 2020
dc913a3
Delete WorkbookGet2.json
tonykslee Nov 20, 2020
5589467
ran prettier
tonykslee Nov 20, 2020
a705d18
Merge branch 'users/tolee/workbooks_s360_compliance' of https://githu…
tonykslee Nov 20, 2020
81ac6cc
added nextLink in list result
tonykslee Nov 20, 2020
7772dd5
removed suppression for test
tonykslee Nov 20, 2020
9aa76af
removed x-ms-enum
tonykslee Nov 20, 2020
f1c3eef
removed x-ms-azure-resource
tonykslee Nov 20, 2020
27139ef
added back the x-ms-azure-resource
tonykslee Nov 20, 2020
ddf88f0
try removiung identity and kind
tonykslee Nov 20, 2020
82bdfd4
Revert "try removiung identity and kind"
tonykslee Nov 20, 2020
b6a287f
removed extra example references
tonykslee Nov 21, 2020
6a82c5c
Revert "removed extra example references"
tonykslee Nov 21, 2020
5f81d03
updated schema to match myworkbooks
tonykslee Nov 21, 2020
9a2a3e7
updated with some missing fields in actual response
tonykslee Nov 24, 2020
a190ed3
added missing etag and marked sourceId as optional
tonykslee Nov 24, 2020
262366e
added readonly to workbookListResult
tonykslee Nov 24, 2020
5cc408a
reinstaed the suppression
tonykslee Dec 3, 2020
ffca810
Merge branch 'master' into users/tolee/workbooks_s360_compliance
tonykslee Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ghts/resource-manager/Microsoft.Insights/stable/2020-10-20/examples/MyWorkbookDelete.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"resourceName": "deadb33f-5e0d-4064-8ebb-1a4ed0313eb2"
},
"responses": {
"201": {},
"200": {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete should return 200 when done successfully rather than 201. So this is a bugfix. We'd suggest you to ask for approval from breaking change review board.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting that I add this to the existing swagger kpi exemption request or to create a new one for this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I suggest you to request breaking change approval for this case. See https://aka.ms/AzBreakingChangesPolicy

"204": {}
}
}
2 changes: 1 addition & 1 deletion ...ghts/resource-manager/Microsoft.Insights/stable/2020-10-20/examples/MyWorkbookUpdate.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
}
},
"responses": {
"200": {
"201": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

201 means Created. For Patch action, it should return 200. So to fix the s360, the service implementation need to be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only updating the swagger documentation to reflect the actual response codes that are being returned form this api. Currently upon PATCH request for this api, it returns a 201 and not a 200.

The way I understand it, the cause for the S360 error was because the actual response was returning a 201, but the existing swagger configuration was expecting a 200 (as you are suggesting it should be). Therefore, this PR changes the response code to 201 to match the existing api response code.

Copy link
Contributor

@leni-msft leni-msft Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are quite many breaking changes, as you may have noticed. Per rule made by breaking change review board, you will need follow the process defined in https://aka.ms/AzBreakingChangesPolicy to request breaking approval. Thus, for this specific issue, I would suggest you to change service implementation instead. That will also resolve the S360 issue, and there's no breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it sounds like I understood the S360 error correctly.

there are many breaking changes because the previous swagger definition was defined incorrectly to start with. that is why i'm requesting a breaking change exemption so that we can start regularly maintaining it and keeping it up to date.

It seemed like the full breaking change approval process would take a long time and since the s360 deadline was approaching, I thought that the exception was the right request since these changes are not actually breaking changes - they are correcting the incorrect existing swagger definition. But if there's no way around it and we have to go through the whole process, then that is fine. I'm just concerned that we'd just be out of SLA for the long duration of the approval process.

"body": {
"id": "c0deea5e-3344-40f2-96f8-6f8e1c3b5722",
"type": "Microsoft.Insights/myworkbooks",
Expand Down
3 changes: 2 additions & 1 deletion ...ights/resource-manager/Microsoft.Insights/stable/2020-10-20/examples/MyWorkbooksList.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@
"storageUri": null
}
}
]
],
"nextLink": null
}
}
}
14 changes: 14 additions & 0 deletions ...ghts/resource-manager/Microsoft.Insights/stable/2020-10-20/examples/MyWorkbooksList2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"parameters": {
"api-version": "2020-10-20",
"subscriptionId": "6b643656-33eb-422f-aee8-3ac145d124af",
"resourceGroupName": "my-resource-group",
"category": "workbook"
},
"responses": {
"200": {
"body": [],
"nextLink": null
}
}
}
2 changes: 1 addition & 1 deletion ...sights/resource-manager/Microsoft.Insights/stable/2020-10-20/examples/WorkbookDelete.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"resourceName": "deadb33f-5e0d-4064-8ebb-1a4ed0313eb2"
},
"responses": {
"201": {},
"200": {},
"204": {}
}
}
2 changes: 1 addition & 1 deletion ...resource-manager/Microsoft.Insights/stable/2020-10-20/examples/WorkbookManagedUpdate.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
}
},
"responses": {
"200": {
"201": {
"body": {
"identity": {
"type": "UserAssigned",
Expand Down
2 changes: 1 addition & 1 deletion ...sights/resource-manager/Microsoft.Insights/stable/2020-10-20/examples/WorkbookUpdate.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
}
},
"responses": {
"200": {
"201": {
"body": {
"id": "c0deea5e-3344-40f2-96f8-6f8e1c3b5722",
"type": "",
Expand Down
3 changes: 2 additions & 1 deletion ...nsights/resource-manager/Microsoft.Insights/stable/2020-10-20/examples/WorkbooksList.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@
"storageUri": null
}
}
]
],
"nextLink": null
}
}
}
97 changes: 61 additions & 36 deletions ...cationinsights/resource-manager/Microsoft.Insights/stable/2020-10-20/myworkbooks_API.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@
{
"$ref": "#/parameters/TagsParameter"
},
{
"$ref": "#/parameters/SourceIdParameter"
},
{
"$ref": "#/parameters/CanFetchWorkbookContentParameter"
},
Expand All @@ -73,11 +76,14 @@
}
},
"x-ms-pageable": {
"nextLinkName": null
"nextLinkName": "nextLink"
},
"x-ms-examples": {
"WorkbooksList": {
"$ref": "./examples/MyWorkbooksList.json"
},
"WorkbooksList2": {
"$ref": "./examples/MyWorkbooksList2.json"
}
}
}
Expand Down Expand Up @@ -118,11 +124,14 @@
}
},
"x-ms-pageable": {
"nextLinkName": null
"nextLinkName": "nextLink"
},
"x-ms-examples": {
"WorkbooksList": {
"$ref": "./examples/MyWorkbooksList.json"
},
"WorkbooksList2": {
"$ref": "./examples/MyWorkbooksList2.json"
}
}
}
Expand Down Expand Up @@ -183,7 +192,7 @@
}
],
"responses": {
"201": {
"200": {
"description": "The private workbook has been successfully deleted."
},
"204": {
Expand Down Expand Up @@ -215,6 +224,9 @@
{
"$ref": "#/parameters/WorkbookResourceNameParameter"
},
{
"$ref": "#/parameters/SourceIdParameter"
},
{
"$ref": "../../../../../common-types/resource-management/v1/types.json#/parameters/ApiVersionParameter"
},
Expand Down Expand Up @@ -267,6 +279,9 @@
{
"$ref": "#/parameters/WorkbookResourceNameParameter"
},
{
"$ref": "#/parameters/SourceIdParameter"
},
{
"$ref": "../../../../../common-types/resource-management/v1/types.json#/parameters/ApiVersionParameter"
},
Expand All @@ -281,7 +296,7 @@
}
],
"responses": {
"200": {
"201": {
"description": "The private workbook definition updated.",
"schema": {
"$ref": "#/definitions/MyWorkbook"
Expand Down Expand Up @@ -346,6 +361,12 @@
"type": "string"
},
"description": "Resource tags"
},
"etag": {
"additionalProperties": {
"type": "string"
},
"description": "Resource etag"
}
},
"x-ms-azure-resource": true,
Expand All @@ -360,6 +381,9 @@
"$ref": "#/definitions/MyWorkbook"
},
"description": "An array of private workbooks."
},
"nextLink": {
"type": "string"
}
},
"description": "Workbook list result."
Expand All @@ -380,7 +404,7 @@
"shared"
],
"x-ms-enum": {
"name": "SharedTypeKind",
"name": "kind",
"modelAsString": true
}
},
Expand All @@ -405,6 +429,7 @@
},
"serializedData": {
"type": "string",
"x-nullable": true,
"description": "Configuration of this particular private workbook. Configuration data is a string containing valid JSON"
},
"version": {
Expand Down Expand Up @@ -438,46 +463,54 @@
},
"storageUri": {
"type": "string",
"x-nullable": true,
"description": "BYOS Storage Account URI"
}
}
},
"ErrorFieldContract": {
"MyWorkbookError": {
"description": "Error response.",
"properties": {
"code": {
"type": "string",
"description": "Property level error code."
},
"message": {
"type": "string",
"description": "Human-readable representation of property-level error."
},
"target": {
"type": "string",
"description": "Property name."
"error": {
"$ref": "#/definitions/ErrorDefinition",
"description": "The error details."
}
},
"description": "Error Field contract."
}
},
"MyWorkbookError": {
"ErrorDefinition": {
"description": "Error definition.",
"properties": {
"code": {
"description": "Service specific error code which serves as the substatus for the HTTP error code.",
"type": "string",
"description": "Service-defined error code. This code serves as a sub-status for the HTTP error code specified in the response."
"readOnly": true
},
"message": {
"description": "Description of the error.",
"type": "string",
"description": "Human-readable representation of the error."
"readOnly": true
},
"details": {
"innererror": {
"description": "Internal error details.",
"items": {
"$ref": "#/definitions/InnerErrorTrace"
},
"readOnly": true
}
}
},
"InnerErrorTrace": {
"description": "Error details",
"properties": {
"trace": {
"description": "detailed error trace",
"type": "array",
"items": {
"$ref": "#/definitions/ErrorFieldContract"
"type": "string"
},
"description": "The list of invalid fields send in request, in case of validation error."
"readOnly": true
}
},
"description": "Error message body that will indicate why the operation failed."
}
},
"ManagedIdentity": {
"description": "Customer Managed Identity",
Expand Down Expand Up @@ -541,19 +574,11 @@
"SourceIdParameter": {
"name": "sourceId",
"in": "query",
"required": true,
"required": false,
"type": "string",
"description": "Azure Resource Id that will fetch all linked workbooks.",
"x-ms-parameter-location": "method"
},
"ResourceIdParameter": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing the definition here? Better keep it to avoid breaking change error, if not necessary,

Copy link
Contributor Author

@tonykslee tonykslee Dec 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't exist and it never has. Apparently the initial definition was incorrect and then still approved. So its been just wrong for many years.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Then actually it's not a breaking change. You may ask for breaking change approval for this case.

"name": "resourceId",
"in": "query",
"required": true,
"type": "string",
"description": "Azure Resource Id or any target workbook resource id.",
"x-ms-parameter-location": "method"
},
"CanFetchWorkbookContentParameter": {
"name": "canFetchContent",
"in": "query",
Expand Down
Loading