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

update roledefinition get custom roles to use filter #5664

Merged
merged 9 commits into from
Mar 13, 2018

Conversation

darshanhs90
Copy link
Contributor

@darshanhs90 darshanhs90 commented Mar 2, 2018

waiting for Azure/azure-sdk-for-net#4102
and #5646
update roledefinition get custom roles to use filter[donot merge]
waiting for sdk-for-net pr to get completed

Description

Checklist

@maddieclayton
Copy link
Contributor

@darshanhs90 Please let me know when the SDK has been merged and you are ready for us to take a look at this PR.

@cormacpayne
Copy link
Member

@darshanhs90 looks like the SDK PR was merged -- was this new SDK version published to NuGet?

@darshanhs90
Copy link
Contributor Author

@cormacpayne Yes it was published on friday,Will update this PR today

@darshanhs90 darshanhs90 changed the title update roledefinition get custom roles to use filter[donot merge] update roledefinition get custom roles to use filter Mar 12, 2018
Copy link
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

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

@darshanhs90 one comment, otherwise LGTM


# Test
[Microsoft.Azure.Commands.Resources.Models.Authorization.AuthorizationClient]::RoleAssignmentNames.Enqueue("2fec1202-19ad-4c8b-b461-6031a1b5dce6")
$newAssignment = New-AzureRmRoleAssignment `
Copy link
Member

Choose a reason for hiding this comment

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

@darshanhs90 why is this command being removed from this test?

Copy link
Contributor Author

@darshanhs90 darshanhs90 Mar 13, 2018

Choose a reason for hiding this comment

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

@cormacpayne it was a redundant thing added in the test,as you can see earlier in the same test,we are creating a roleassignment, so that would be sufficient to validate the scenario for this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad,thought the earlier create was also for a roleassignment, will revert it

@cormacpayne
Copy link
Member

@cormacpayne cormacpayne merged commit a3ee9d6 into Azure:preview Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants