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

Add sample for custom role definition and assignment #1045

Merged
merged 2 commits into from
Dec 1, 2020

Conversation

wilfriedwoivre
Copy link
Contributor

No description provided.

@wilfriedwoivre wilfriedwoivre changed the title Add sample for custom role and assignment Add sample for custom role definition and assignment Nov 30, 2020
Copy link
Collaborator

@alex-frankel alex-frankel left a comment

Choose a reason for hiding this comment

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

small tweak

}

resource assignment 'Microsoft.Authorization/roleAssignments@2020-04-01-preview' = {
name: guid(concat(roleName, principalId))
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of concatting these strings together, I would add arguments to the guid function (which can take as many arguments as you want to provide). I would also add the subscription id as an argument to ensure uniqueness:

name: guid(roleName, principalId, subscription().subscriptionId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea !
I make the small tweak

@codecov-io
Copy link

Codecov Report

Merging #1045 (98d76c3) into main (b127e55) will increase coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1045      +/-   ##
==========================================
+ Coverage   94.26%   94.73%   +0.47%     
==========================================
  Files         328      324       -4     
  Lines       15810    15660     -150     
  Branches       12        0      -12     
==========================================
- Hits        14903    14836      -67     
+ Misses        907      824      -83     
Flag Coverage Δ
dotnet 94.73% <ø> (-0.09%) ⬇️
typescript ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...cep.Core.UnitTests/FileSystem/FileResolverTests.cs 79.31% <0.00%> (-20.69%) ⬇️
.../Bicep.Core.UnitTests/Assertions/BaselineHelper.cs 63.63% <0.00%> (-15.91%) ⬇️
src/Bicep.Core/FileSystem/PathHelper.cs 96.15% <0.00%> (-3.85%) ⬇️
src/Bicep.Core.UnitTests/Utils/CompilerFlags.cs 33.33% <0.00%> (ø)
...Bicep.Core.UnitTests/FileSystem/PathHelperTests.cs 100.00% <0.00%> (ø)
src/vscode-bicep/src/utils/logger.ts
src/vscode-bicep/src/language/client.ts
src/vscode-bicep/src/utils/telemetry.ts
src/vscode-bicep/src/extension.ts
src/Bicep.Core.Samples/ExamplesTests.cs 98.73% <0.00%> (+0.01%) ⬆️

@alex-frankel alex-frankel merged commit 807e442 into Azure:main Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants