Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Managed instance data classification REST API - examples files name fixes #3673

Merged
merged 20 commits into from
Sep 25, 2018
Merged

Managed instance data classification REST API - examples files name fixes #3673

merged 20 commits into from
Sep 25, 2018

Conversation

igsiroti
Copy link
Contributor

No description provided.

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Aug 20, 2018

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

@AutorestCI
Copy link

AutorestCI commented Aug 20, 2018

Automation for azure-sdk-for-ruby

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-ruby#1562

@AutorestCI
Copy link

AutorestCI commented Aug 20, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI
Copy link

AutorestCI commented Aug 20, 2018

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@AutorestCI
Copy link

AutorestCI commented Aug 20, 2018

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#3483

@annatisch
Copy link
Member

Thanks @igsiroti!
The changes look fine to me - however there are number of warnings in the breaking change CI that appear inconsistent with the diff in this PR.
Are you able to shed some light on these?

{
  "id": "1005",
  "code": "RemovedPath",
  "message": "The new version is missing a path that was found in the old version. Was path '/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName}/sensitivityLabels' removed or restructured?",
  "jsonref": "#/paths/~1subscriptions~1{subscriptionId}~1resourceGroups~1{resourceGroupName}~1providers~1Microsoft.Sql~1servers~1{serverName}~1databases~1{databaseName}~1sensitivityLabels",
  "json-path": "#/paths/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName}/sensitivityLabels",
  "type": "Warning"
}

{
  "id": "1005",
  "code": "RemovedPath",
  "message": "The new version is missing a path that was found in the old version. Was path '/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName}/sensitivityLabels/{sensitivityLabelSource}' removed or restructured?",
  "jsonref": "#/paths/~1subscriptions~1{subscriptionId}~1resourceGroups~1{resourceGroupName}~1providers~1Microsoft.Sql~1servers~1{serverName}~1databases~1{databaseName}~1sensitivityLabels~1{sensitivityLabelSource}",
  "json-path": "#/paths/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName}/sensitivityLabels/{sensitivityLabelSource}",
  "type": "Warning"
}

{
  "id": "1005",
  "code": "RemovedPath",
  "message": "The new version is missing a path that was found in the old version. Was path '/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName}/schemas/{schemaName}/tables/{tableName}/columns/{columnName}/sensitivityLabels/{sensitivityLabelSource}' removed or restructured?",
  "jsonref": "#/paths/~1subscriptions~1{subscriptionId}~1resourceGroups~1{resourceGroupName}~1providers~1Microsoft.Sql~1servers~1{serverName}~1databases~1{databaseName}~1schemas~1{schemaName}~1tables~1{tableName}~1columns~1{columnName}~1sensitivityLabels~1{sensitivityLabelSource}",
  "json-path": "#/paths/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName}/schemas/{schemaName}/tables/{tableName}/columns/{columnName}/sensitivityLabels/{sensitivityLabelSource}",
  "type": "Warning"
}

{
  "id": "1038",
  "code": "AddedPath",
  "message": "The new version is adding a path that was not found in the old version.",
  "jsonref": "#/paths/~1subscriptions~1{}~1resourceGroups~1{}~1providers~1Microsoft.Sql~1managedInstances~1{}~1databases~1{}~1schemas~1{}~1tables~1{}~1columns~1{}~1sensitivityLabels~1{}",
  "json-path": "#/paths/subscriptions/{}/resourceGroups/{}/providers/Microsoft.Sql/managedInstances/{}/databases/{}/schemas/{}/tables/{}/columns/{}/sensitivityLabels/{}",
  "type": "Info"
}

{
  "id": "1038",
  "code": "AddedPath",
  "message": "The new version is adding a path that was not found in the old version.",
  "jsonref": "#/paths/~1subscriptions~1{}~1resourceGroups~1{}~1providers~1Microsoft.Sql~1managedInstances~1{}~1databases~1{}~1sensitivityLabels",
  "json-path": "#/paths/subscriptions/{}/resourceGroups/{}/providers/Microsoft.Sql/managedInstances/{}/databases/{}/sensitivityLabels",
  "type": "Info"
}

{
  "id": "1038",
  "code": "AddedPath",
  "message": "The new version is adding a path that was not found in the old version.",
  "jsonref": "#/paths/~1subscriptions~1{}~1resourceGroups~1{}~1providers~1Microsoft.Sql~1managedInstances~1{}~1databases~1{}~1sensitivityLabels~1{}",
  "json-path": "#/paths/subscriptions/{}/resourceGroups/{}/providers/Microsoft.Sql/managedInstances/{}/databases/{}/sensitivityLabels/{}",
  "type": "Info"
}

{
  "id": "1007",
  "code": "RemovedClientParameter",
  "message": "The new version is missing a client parameter that was found in the old version. Was 'ServerNameParameter' removed or renamed?",
  "jsonref": "#/parameters",
  "json-path": "#/parameters",
  "type": "Warning"
}

@igsiroti
Copy link
Contributor Author

Thanks @annatisch for the review! In the removed files there was a typo in their names '..Sensitivty...' instead of '..Sensitivity...' so I renamed them (it shows as if removed and added) and updated the refs. Does it makes sense?

@annatisch
Copy link
Member

Thanks @igsiroti - yes I understand the changes in the PR.
My question was more regarding the warnings I printed in my last post - it's showing path changes, for example:
"#/paths/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName}/sensitivityLabels"
has been changed to:
"#/paths/subscriptions/{}/resourceGroups/{}/providers/Microsoft.Sql/managedInstances/{}/databases/{}/sensitivityLabels"

And the serverName client parameter used in the original URL is no longer present.

Were these path changes recently introduced to the spec? Just trying to figure out where these warnings are coming from :)

@igsiroti
Copy link
Contributor Author

sorry my bad. adding it now

@annatisch
Copy link
Member

Thanks @igsiroti - though I'm a bit confused - are these new changes to the API? Were they existing changes that were somehow reverted?

@annatisch
Copy link
Member

Looking at the changes it appears that:

  • Some new breaking APIs that were added to sensitivityLabels.json have been reverted which resolves the breaking changes.
  • These new APIs have now been moved to managedInstanceSensitivityLabels.json and will coexist.

Does this sound accurate?
Can you confirm whether these new managedInstanceSensitivityLabels APIs have already received ARM signoff and if so could you please link to the PR?

Finally - are you aware that currently no SDKs are being generated for these new APIs? If you wish for them to be added to the SDK, please add the new spec to the package tag in the readme :)

Thanks!

@igsiroti
Copy link
Contributor Author

  • That is correct

  • The APIs haven't received ARM signoff yet

  • Yes I'm aware of that

Thanks

@annatisch annatisch added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 27, 2018
@annatisch
Copy link
Member

Thanks @igsiroti!
Adding @ravbhatnagar for ARM review.

@annatisch annatisch requested a review from ravbhatnagar August 27, 2018 14:08
@annatisch
Copy link
Member

Ping @ravbhatnagar

Removed sensitivityLabelSource segment. Changed REST API to (in logical server and managed instance(:
providers/Microsoft.Sql/managedInstances/{managedInstanceName}/databases/{databaseName}/sensitivityLabels
/providers/Microsoft.Sql/managedInstances/{managedInstanceName}/databases/{databaseName}/currentSensitivityLabels
providers/Microsoft.Sql/managedInstances/{managedInstanceName}/databases/{databaseName}/recommendedSensitivityLabels
@annatisch
Copy link
Member

Pinging @ravbhatnagar again - could you please take a look at the new managed instance sensitivity label APIs?

"$ref": "#/definitions/SensitivityLabel"
}
},
"default": {
Copy link
Contributor

Choose a reason for hiding this comment

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

schema for the default response much be the ARM error contract as described in RPC. Check Batch RP swagger for a sample.

Copy link
Contributor

Choose a reason for hiding this comment

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

checked with Igal. He will follow up with Jared since all SQL swaggers are generated by tool.

@ravbhatnagar
Copy link
Contributor

@igsiroti FYI

@annatisch
Copy link
Member

Thanks @ravbhatnagar
@igsiroti - what is the status of this PR?
Are you waiting on input from a 3rd party regarding the ARM error responses in the spec?

@igsiroti
Copy link
Contributor Author

Correct. I want to make sure we are OK with the default response @ravbhatnagar mentioned above. Waiting for Jared's response

@ravbhatnagar
Copy link
Contributor

Default response can be added later as it needs to be added to all SQL APIs. Signing off on this from ARM side.

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMReviewInProgress WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Sep 17, 2018
"name": "sensitivityLabelSource",
"in": "path",
"description": "Optional source of the sensitivity label. Valid values are current or recommeneded. In not specified both returned.",
"required": true,
Copy link
Member

Choose a reason for hiding this comment

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

The description says this is optional, but it's marked as required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how but it's old file. Fixing it

Copy link
Member

Choose a reason for hiding this comment

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

You're right - looks like I didn't have the latest changes :)

@AutorestCI
Copy link

AutorestCI commented Sep 20, 2018

Automation for azure-sdk-for-js

Nothing to generate for azure-sdk-for-js

@annatisch
Copy link
Member

annatisch commented Sep 20, 2018

Thanks @igsiroti - it's looking good.
Last thing - You changed the names of the operations to "ManagedDatabaseSensitivityLabels" yet the spec itself is still called "ManagedInstanceSensitivityLabels.json" - could this be renamed?

@igsiroti
Copy link
Contributor Author

Thanks @annatisch. I renamed the spec

@annatisch annatisch merged commit 57d827e into Azure:master Sep 25, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants