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

keyvault soft-delete #8956

Closed
wants to merge 1 commit into from
Closed

Conversation

isra-fel
Copy link
Member

@isra-fel isra-fel commented Dec 2, 2019

No description provided.

@isra-fel isra-fel requested a review from erich-wang as a code owner December 2, 2019 06:21
@isra-fel isra-fel self-assigned this Dec 2, 2019
@isra-fel isra-fel added the Mgmt This issue is related to a management-plane library. label Dec 2, 2019
@@ -54,7 +54,7 @@ public KeyVaultTestBase(MockContext context)
var graphClient = context.GetServiceClient<GraphRbacManagementClient>();
graphClient.TenantID = this.tenantId;
graphClient.BaseUri = testEnv.Endpoints.GraphUri;
this.objectId = graphClient.User.Get(testEnv.UserName).ObjectId;
this.objectId = testEnv.UserName;
Copy link
Member

@schaabs schaabs Dec 2, 2019

Choose a reason for hiding this comment

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

I'm not sure of the reasoning behind this change. I assume that the object id is being used to construct access policies. In this case the object id is not interchangeable with username. Perhaps the way the test environment is set up it is setting UserName to the users object id? In this case should the property be renamed to avoid confusion?

Copy link
Member Author

@isra-fel isra-fel Dec 3, 2019

Choose a reason for hiding this comment

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

@schaabs Glad you reviewed! In fact, I couldn't get it working, graphClient.User.Get() always threw an cloud exception without meaningful error message.
That's why I chose not to call graph, but pass in object ID directly.

Could you share if there are any special prerequisites required? Here are the environment variables I used:

set TEST_CSM_ORGID_AUTHENTICATION=SubscriptionId=***;ServicePrincipal=***;ServicePrincipalSecret=***;AADTenant=***;Environment=Prod;HttpRecorderMode=Record;UserId=***;
set AZURE_TEST_MODE=Record

Thanks

@isra-fel isra-fel closed this Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants