Skip to content
Merged
Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
7c3cfc5
constructor and some of the surrounding things
sadasant Aug 24, 2020
28b4eb6
createRoleAssignment
sadasant Aug 24, 2020
eb9cbe0
createRoleAssignment documentation
sadasant Aug 24, 2020
a5524f5
deleteRoleAssignment
sadasant Aug 24, 2020
167935d
getRoleAssignment
sadasant Aug 24, 2020
44e9eb3
listRoleAssignments
sadasant Aug 24, 2020
3790bd1
listRoleDefinitions
sadasant Aug 24, 2020
2188a87
api-extractor
sadasant Aug 24, 2020
60f4bd3
formatting
sadasant Aug 24, 2020
ac414c9
deleted the filter option on the list operations
sadasant Aug 24, 2020
9e5505a
fixing the build
sadasant Aug 24, 2020
57fd102
lint fixes
sadasant Aug 24, 2020
0ce383e
forgot to api extract the last commit
sadasant Aug 24, 2020
7f179ef
bad reference to a parameter
sadasant Aug 25, 2020
8919a93
Resolves https://github.com/Azure/azure-sdk-for-js/pull/10815#discuss…
sadasant Aug 25, 2020
ee1d71e
Addressing: https://github.com/Azure/azure-sdk-for-js/pull/10815#disc…
sadasant Aug 25, 2020
b7d3bf5
Feedback fixes
sadasant Aug 25, 2020
26e9583
role to roleScope, and RoleDefinitionPermission to KeyVaultPermission
sadasant Aug 26, 2020
a10a592
KeyVaultAccessControlClient documentation and removing lint from the …
sadasant Aug 26, 2020
09148fd
cleaned up bad reference to KeyClient
sadasant Aug 26, 2020
1b0f642
Addressing https://github.com/Azure/azure-sdk-for-js/pull/10815#discu…
sadasant Aug 27, 2020
4513d72
Merge remote-tracking branch 'Azure/master' into keyvault-admin/10799…
sadasant Aug 27, 2020
6ab4ae1
fixed bad references
sadasant Aug 27, 2020
7c845ca
Update sdk/keyvault/keyvault-admin/src/accessControlClient.ts
sadasant Sep 1, 2020
9030441
Apply suggestions from code review
sadasant Sep 2, 2020
18d7911
Fixes after updating from remote
sadasant Sep 2, 2020
74b7bfc
Addressing https://github.com/Azure/azure-sdk-for-js/pull/10815/files…
sadasant Sep 2, 2020
ca12be4
tsdoc cleanup
sadasant Sep 2, 2020
e306ff4
linting
sadasant Sep 2, 2020
d61f0b6
Merge remote-tracking branch 'Azure/master' into keyvault-admin/10799…
sadasant Sep 2, 2020
64a5d1d
fixes after updating the generated files
sadasant Sep 2, 2020
1386365
enforcing most of the properties, after discussing with .Net
sadasant Sep 2, 2020
590a835
Addressing https://github.com/Azure/azure-sdk-for-js/pull/10815#discu…
sadasant Sep 2, 2020
23431cc
no more operationOptionsToRequestOptionsBase
sadasant Sep 4, 2020
66bcdd4
Key Vault
sadasant Sep 4, 2020
8484177
Merge remote-tracking branch 'Azure/master' into keyvault-admin/10799…
sadasant Sep 4, 2020
24380a7
only 7.2-preview service version
sadasant Sep 4, 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
Prev Previous commit
Next Next commit
forgot to api extract the last commit
  • Loading branch information
sadasant committed Aug 24, 2020
commit 0ce383e9ecf53989d8e9f9e468d2cd213fb6b7b7
6 changes: 5 additions & 1 deletion sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export class AccessControlClient {
constructor(vaultUrl: string, credential: TokenCredential, pipelineOptions?: AccessControlClientOptions);
createRoleAssignment(scope: RoleAssignmentScope, name: string, options?: CreateRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;
Copy link
Member

Choose a reason for hiding this comment

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

Per discussion below, the principalId and roleDefinitionId (?) should be parameters. Same goes for any required parameters in other methods that follow.

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 have updated this by this point, to be:

    createRoleAssignment(scope: RoleAssignmentScope, name: string, roleDefinitionId: string, principalId: string, options?: CreateRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;

Note that I used roleDefinitionId first, then principalId, since I believe that the former one has a smaller scope than the later one, so I'm trying to go from micro to macro, scope-wise. Does this make sense? I can change it, I won't push back, but this is my argument for my current approach.

Wait, what other methods have these as the required parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only methods that have these properties in .Net are: CreateRoleAssignment, and CreateRoleAssignmentAsync.

deleteRoleAssignment(scope: RoleAssignmentScope, name: string, options?: DeleteRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;
getRoleAssignment(scope: RoleAssignmentScope, name: string, options?: DeleteRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;
getRoleAssignment(scope: RoleAssignmentScope, name: string, options?: GetRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;
listRoleAssignments(scope: RoleAssignmentScope, options?: ListRoleAssignmentsOptions): PagedAsyncIterableIterator<KeyVaultRoleAssignment>;
listRoleDefinitions(scope: RoleAssignmentScope, options?: ListRoleDefinitionsOptions): PagedAsyncIterableIterator<KeyVaultRoleDefinition>;
readonly vaultUrl: string;
Expand All @@ -33,6 +33,10 @@ export interface CreateRoleAssignmentOptions extends coreHttp.OperationOptions {
export interface DeleteRoleAssignmentOptions extends coreHttp.OperationOptions {
}

// @public
export interface GetRoleAssignmentOptions extends coreHttp.OperationOptions {
}

// @public
export interface KeyVaultRoleAssignment {
Copy link
Member

Choose a reason for hiding this comment

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

Just RoleAssignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I do this change, let's argue about prefixes here: #10815 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heaths I'm assuming KeyVaultRoleAssignment is favored after that conversation I mentioned ^

Choose a reason for hiding this comment

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

python has KeyVaultRoleAssignment

readonly id?: string;
Expand Down