[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient #10815

sadasant · 2020-08-24T22:57:07Z

Convenience layer for the AccessControlClient.
After this PR, I'll make another one for the BackupClient.

You can see the rendered version of the latest API review file by going to this link: https://github.com/sadasant/azure-sdk-for-js/blob/keyvault-admin/10799-convenience-layer/sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

This API is based on the .Net's API: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/keyvault/Azure.Security.KeyVault.Administration/api/Azure.Security.KeyVault.Administration.netstandard2.0.cs (with some language-specific considerations).

IMPORTANT:

For .Net, this client is called KeyVaultAccessControlClient. Since none of the other clients in KeyVault start with KeyVault (as in, KeyClient), it seemed appropriate to name ours AccessControlClient. Agreed on naming it KeyVaultAccessControlClient.
I'm intentionally not using the spread operator, to ensure the translation of properties is as clear as possible.
I had to enforce a bunch of properties of several types. The swagger doesn't say so, but they seem to be required by the service, and other languages are doing the same.

Many things are still missing from the keyvault-admin folder, so keep in mind that this is an incremental effort.

Part of #10799
Closes #7279

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

sdk/keyvault/keyvault-admin/src/accessControlClient.ts

xirzec

The convenience client looks great!

I left some minor API feedback and a few code nits.

xirzec · 2020-09-01T22:30:32Z

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

+}
+
+// @public
+export type RoleAssignmentScope = "/" | "/keys" | string;


this feels like a weird type, though I'm not sure what would be better

The string part is because it accepts UUIDs. I wonder if we could have some UUID validator as a type? That would be generally helpful in our clients.

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

sdk/keyvault/keyvault-admin/src/accessControlClient.ts

sdk/keyvault/keyvault-admin/src/accessControlModels.ts

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

…/6ab4ae11154ef9b118002bed7def865cf7fd0226#r481471012

Addresses: - https://github.com/Azure/azure-sdk-for-js/pull/10815/files#r481471259 - https://github.com/Azure/azure-sdk-for-js/pull/10815/files#r481471382

…-convenience-layer

sdk/keyvault/keyvault-admin/src/accessControlClient.ts

The generated code already has this now!

…-convenience-layer

iscai-msft

Mainly left comments on where python differs, not saying what we're doing is correct htough

iscai-msft · 2020-09-04T14:57:48Z

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

+// @public
+export class KeyVaultAccessControlClient {
+    constructor(vaultUrl: string, credential: TokenCredential, pipelineOptions?: AccessControlClientOptions);
+    createRoleAssignment(roleScope: RoleAssignmentScope, name: string, roleDefinitionId: string, principalId: string, options?: CreateRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;


in python we have it as role_assignment_name

same for every name parameter for role assignments

.Net is using name. I don't mind, but I'd rather reach to an agreement with @heaths , @christothes

iscai-msft · 2020-09-04T14:59:20Z

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

+}
+
+// @public
+export interface KeyVaultRoleAssignment {


python has KeyVaultRoleAssignment

iscai-msft · 2020-09-04T14:59:52Z

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

+
+// @public
+export interface KeyVaultRoleAssignment {
+    readonly id: string;


for python we have assignment_id

.Net has Id: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/keyvault/Azure.Security.KeyVault.Administration/api/Azure.Security.KeyVault.Administration.netstandard2.0.cs#L121 cc: @christothes what should we use? who should we ask?

IMO - id is contextually a property of the KeyVaultRoleAssignment and shouldn't need a prefix.

iscai-msft · 2020-09-04T15:00:48Z

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

+}
+
+// @public
+export interface KeyVaultRoleDefinition {


python has KeyVaultRoleDefinition

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md

sdk/keyvault/keyvault-admin/src/constants.ts

sadasant added 9 commits August 24, 2020 17:41

constructor and some of the surrounding things

7c3cfc5

createRoleAssignment

28b4eb6

createRoleAssignment documentation

eb9cbe0

deleteRoleAssignment

a5524f5

getRoleAssignment

167935d

listRoleAssignments

44e9eb3

listRoleDefinitions

3790bd1

api-extractor

2188a87

formatting

60f4bd3

sadasant self-assigned this Aug 24, 2020

ghost added the KeyVault label Aug 24, 2020

deleted the filter option on the list operations

ac414c9

sadasant requested a review from christothes August 24, 2020 23:03

sadasant added 3 commits August 24, 2020 23:09

fixing the build

9e5505a

lint fixes

57fd102

forgot to api extract the last commit

0ce383e

sadasant marked this pull request as ready for review August 25, 2020 10:41

sadasant requested review from bterlson, sophiajt and xirzec as code owners August 25, 2020 10:41

sadasant requested review from schaabs, g2vinay and chlowell August 25, 2020 10:42

christothes reviewed Aug 25, 2020

View reviewed changes

sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md Outdated Show resolved Hide resolved

christothes reviewed Aug 25, 2020

View reviewed changes

sdk/keyvault/keyvault-admin/src/accessControlClient.ts Outdated Show resolved Hide resolved

christothes reviewed Aug 25, 2020

View reviewed changes

sdk/keyvault/keyvault-admin/src/accessControlClient.ts Show resolved Hide resolved

christothes reviewed Aug 25, 2020

View reviewed changes

sdk/keyvault/keyvault-admin/src/accessControlClient.ts Outdated Show resolved Hide resolved

sadasant requested a review from heaths August 25, 2020 17:03

sadasant added 2 commits August 25, 2020 17:43

bad reference to a parameter

7f179ef

Resolves Azure#10815 (comment)

8919a93

sadasant changed the title ~~[Key Vault Admin] Convenience layer - AccessControlClient~~ [Key Vault Admin] Convenience layer - KeyVaultAccessControlClient Aug 27, 2020

sadasant mentioned this pull request Aug 27, 2020

[Key Vault Admin] Convenience layer - KeyVaultBackupClient sadasant/azure-sdk-for-js#20

Closed

fixed bad references

6ab4ae1

This was referenced Sep 1, 2020

[keyvault-admin] Support Role-based Access Control (RBAC) for data plane via new KeyVaultAccessControlClient #7279

Closed

[keyvault-admin] Implement a new client with vault backup and restore operations #9215

Closed

sadasant requested a review from vcolin7 September 1, 2020 16:11

xirzec reviewed Sep 1, 2020

View reviewed changes

sadasant and others added 8 commits September 1, 2020 19:02

Update sdk/keyvault/keyvault-admin/src/accessControlClient.ts

7c845ca

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

Apply suggestions from code review

9030441

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

Fixes after updating from remote

18d7911

Addressing https://github.com/Azure/azure-sdk-for-js/pull/10815/files…

74b7bfc

…/6ab4ae11154ef9b118002bed7def865cf7fd0226#r481471012

tsdoc cleanup

ca12be4

Addresses: - https://github.com/Azure/azure-sdk-for-js/pull/10815/files#r481471259 - https://github.com/Azure/azure-sdk-for-js/pull/10815/files#r481471382

linting

e306ff4

Merge remote-tracking branch 'Azure/master' into keyvault-admin/10799…

d61f0b6

…-convenience-layer

fixes after updating the generated files

64a5d1d

xirzec reviewed Sep 2, 2020

View reviewed changes

sdk/keyvault/keyvault-admin/src/accessControlClient.ts Outdated Show resolved Hide resolved

sadasant added 2 commits September 2, 2020 17:41

enforcing most of the properties, after discussing with .Net

1386365

Addressing Azure#10815 (comment)

590a835

christothes approved these changes Sep 2, 2020

View reviewed changes

sadasant requested a review from xirzec September 2, 2020 22:32

This was referenced Sep 3, 2020

[Key Vault Admin] Convenience layer - KeyVaultBackupClient #11009

Merged

[Key Vault Admin] Tests #11010

Merged

sadasant added 3 commits September 4, 2020 08:35

no more operationOptionsToRequestOptionsBase

23431cc

The generated code already has this now!

Key Vault

66bcdd4

Merge remote-tracking branch 'Azure/master' into keyvault-admin/10799…

8484177

…-convenience-layer

iscai-msft reviewed Sep 4, 2020

View reviewed changes

only 7.2-preview service version

24380a7

iscai-msft approved these changes Sep 4, 2020

View reviewed changes

sadasant merged commit ab6907c into Azure:master Sep 4, 2020

sadasant deleted the keyvault-admin/10799-convenience-layer branch September 4, 2020 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient #10815

[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient #10815

sadasant commented Aug 24, 2020 •

edited

Loading

xirzec left a comment

xirzec Sep 1, 2020

sadasant Sep 2, 2020

iscai-msft left a comment

iscai-msft Sep 4, 2020

iscai-msft Sep 4, 2020

sadasant Sep 4, 2020

iscai-msft Sep 4, 2020

iscai-msft Sep 4, 2020

sadasant Sep 4, 2020

christothes Sep 4, 2020

iscai-msft Sep 4, 2020

[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient #10815

[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient #10815

Conversation

sadasant commented Aug 24, 2020 • edited Loading

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iscai-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant commented Aug 24, 2020 •

edited

Loading