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.

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

Initial KeyVaultAccessControlClient for Java #12405

Merged
merged 43 commits into from
Sep 4, 2020

Conversation

vcolin7
Copy link
Member

@vcolin7 vcolin7 commented Jun 23, 2020

Tests and samples are still pending and the README is a work in progress.

Based on issue: #8007.

@vcolin7 vcolin7 added KeyVault Client This issue points to a problem in the data-plane of the library. labels Jun 23, 2020
@vcolin7 vcolin7 added this to the [2020] July milestone Jun 23, 2020
@vcolin7 vcolin7 requested review from heaths and g2vinay June 23, 2020 00:11
@vcolin7 vcolin7 self-assigned this Jun 23, 2020
@vcolin7 vcolin7 force-pushed the users/vicolina/keyvault-rbac branch from b0635bc to 0b1fd31 Compare June 24, 2020 20:58
@vcolin7 vcolin7 requested a review from christothes June 25, 2020 18:11
@vcolin7 vcolin7 marked this pull request as ready for review June 25, 2020 18:11
Copy link
Member

@JimSuplizio JimSuplizio left a comment

Choose a reason for hiding this comment

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

There's a tag that needs to be updated in the README that I called out but otherwise the EngSys items in here are fine. I trust you to update the tag so I'll pre-approve this from an EngSys perspective.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Some parameter names different from other languages, but we can unify on those for beta 2. Same for a few getters I pointed out.

eng/jacoco-test-coverage/pom.xml Outdated Show resolved Hide resolved
* @param principalId The principal ID assigned to the role. This maps to the ID inside the Active Directory.
* It can point to a user, service principal, or security group.
*/
public KeyVaultRoleAssignmentProperties(String roleDefinitionId, String principalId) {
Copy link
Member

Choose a reason for hiding this comment

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

These are required parameters for create methods. Are they being passed to those methods directly as well?

Copy link
Member Author

@vcolin7 vcolin7 Sep 2, 2020

Choose a reason for hiding this comment

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

These parameters are passed directly to the service methods.

This Properties object is based on how the service Swagger groups parameters, however, I believe we certainly could change the method signature to take all required values like this:

public Mono<KeyVaultRoleAssignment> createRoleAssignment(
    KeyVaultRoleAssignmentScope roleScope,
    String roleDefinitionId,
    String principalId)

Instead of what we have today:

public Mono<KeyVaultRoleAssignment> createRoleAssignment(
    KeyVaultRoleAssignmentScope scope,
    KeyVaultRoleAssignmentProperties properties)

What do you think @heaths?

Copy link
Member

Choose a reason for hiding this comment

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

I thought we were doing it that way in .NET after a discussion, but I don't see that in source. @christothes and @sadasant, was it a different method we were talking about pulling roleDefinitionId and principalId out of, or was that for the model constructor? I may just be remembering wrong here.

Copy link
Member

Choose a reason for hiding this comment

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

I think we still use a properties bag in .NET, but I like the individual parameter shape better.

@vcolin7 vcolin7 force-pushed the users/vicolina/keyvault-rbac branch from 9ed0f6f to 5a208bf Compare September 3, 2020 22:09
@vcolin7 vcolin7 force-pushed the users/vicolina/keyvault-rbac branch from 8ede798 to ca96cc7 Compare September 4, 2020 06:59
@vcolin7 vcolin7 force-pushed the users/vicolina/keyvault-rbac branch from ca96cc7 to a476010 Compare September 4, 2020 07:03
@vcolin7 vcolin7 merged commit 5eb4798 into master Sep 4, 2020
@vcolin7 vcolin7 deleted the users/vicolina/keyvault-rbac branch September 4, 2020 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants