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

set_role_definition for Key Vault should reorder parameters #18579

Closed
heaths opened this issue May 7, 2021 · 3 comments
Closed

set_role_definition for Key Vault should reorder parameters #18579

heaths opened this issue May 7, 2021 · 3 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Milestone

Comments

@heaths
Copy link
Member

heaths commented May 7, 2021

Based on our API unification review, Python should reorder parameters accordingly:

  1. role_scope : required KeyVaultRoleScope
  2. role_definition_name : optional UUID - autogenerate if None, but document that if customers don't provide a fixed value of an existing definition, a new one will be created.
  3. role_name : optional string - friendly display name; document that it defaults to role_definition_name if absent
  4. description : optional string - long-form description of role definition
  5. permissions : optional list of KeyVaultPermission
  6. assignable_scopes : optional list of KeyVaultRoleScope

We'll use this issue to track any changes to the API from other languages' issues. .NET, Java, and JavaScript should consider a KeyVaultRoleDefinitionParameters as an options bag given the number of possible arguments. Currently, the "Parameters" suffix is the guideline if it contains required parameters, but there is some question about this.

/cc @KrzysztofCwalina @tg-msft @annatisch @johanste @JonathanGiles @bterlson

@heaths
Copy link
Member Author

heaths commented May 7, 2021

Alternatively, we could all probably unify on making the first two parameters positional arguments in the method, and the rest in an options bag/kwargs which should be in line with our respective guidelines. The first two are the only two that are really required (the second is required, but we can / will autogenerate). Thoughts?

@heaths
Copy link
Member Author

heaths commented May 7, 2021

After discussing with an architect, our best course of action for now would be to create an "Options" class like RoleDefinitionOptions or even CreateOrUpdateRoleDefinitionOptions, which follows our normal .NET guidelines.

Having separate constructors/builders would also help make the documented behavior regarding roleDefinitionName more obvious. Thus, we'd have (using .NET as an example):

public class CreateOrUpdateRoleDefinitionOptions
{
  public RoleDefinitionOptions(KeyVaultRoleScope roleScope);
  public RoleDefinitionOptions(KeyVaultRoleScope roleScope, Guid roleDefinitionName);
  public KeyVaultRoleScope RoleScope {get;}
  public Guid RoleDefinitionName {get;}
  // getters and setters on everything else
}
public class KeyVaultAccessControlClient
{
  public Response<KeyVaultRoleDefinition> CreateOrUpdateRoleDefinition(CreateOrUpdateRoleDefinitionOptionsoptions, CancellationToken cancellationToken = default);
  public Response<KeyVaultRoleDefinition> CreateOrUpdateRoleDefinition(KeyVaultRoleScope roleScope, Guid? roleDefinitionName = null, CancellationToken cancellationToken = default);
}

@JonathanGiles
Copy link
Member

The code in your previous comment is suitable for translation and usage in Java too.

@mccoyp mccoyp self-assigned this May 11, 2021
@mccoyp mccoyp closed this as completed Jun 18, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
Development

No branches or pull requests

3 participants