-
Notifications
You must be signed in to change notification settings - Fork 228
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 Licensing Information to AAD Report #954
Conversation
cc792cc
to
2553aeb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to these 2 minor comments, can you also:
-
Update the stub in the unit test (c.f. New-Report.Tests.ps1) to populate SKU related info so that these code paths get minimally tested for not crashing?
-
Make sure the smoke test (esp. SmokeTest002.Tests.ps1) still passes and update it to ensure that the existence of the license table is checked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good flag. I would like us to fix this so we don't ship JS errors. |
* Break setup dependency * debug * Remove MockWith * WIP * Use module name on invoke * debug * debug * debug * debug * debug * wip * wip * wip * wip * Mock IMport-Module * WIP * wip * WIP * Mock OPA Cmd * Rework ScubaConfig test due to rebase * Address warnings in UT
* Update dependencies * Add init for scuba * Remove SkipUpdate * Relpace OPA.ps1 * Call Initialize-SCuBA * Update README * Update PowerShell/ScubaGear/Dependencies.ps1 Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com> * Update README.md Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * Update readme * Update README.md Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com> --------- Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com> Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding feedback based on running and reviewing the results of the report. Overall, looks good and no errors. I did provide feedback on possible grammatical changes and added capability, possibly as future TODO.
@dagarwal-mitre Just letting you know that I tested this as well and it worked fine for me. Nice work! I'd like you to change the title of the new table from "Licensing Information" to "Tenant Licensing Information". |
* Re-pin Graph to latest major/minor version * Re-pin Graph to latest major/minor version * Fix UT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Cmdlet Get-MgBetaSubscribedSku is being called twice in the AAD provider resulting in two calls to the REST API. Please modify the code so that it is only called once. Once the REST API is called, you can save the API results to a new local variable and then still perform the local filtering with Where-Object and Select-Object against the new local variable.
Also move the LicenseInfo variable further up into the code so that it is right after the line containing the ServicePlans variable. This way all licensing related code is grouped in the same block.
* make consistent names and add if condition * update filenames in pipeline * fix filename in pipeline * change workflow names * update names
* Adjust path for required versions * Use Split-Path * throw exception if RequiredVersions not found * throw exception if RequiredVersions not found * add try catch
* rebasing * finish rebase * debug * debug * debug * debug * debug * debug * debug * debug * add key * fix alias * fix thumbprint name * debug * debug * debug * debug * debug * fix typo * debug * check path location * Remove psd1 file * fix path * Remove comments * keep 1 file * stop using dev params
* Tweak comments * Create Merge-JsonOutput function * Undo pds1 file modification * Correct config bug for OutJsonFileName * Mock Merge-JsonOutput function in the RunCached test * Modify pester tests * Remove trailing whitespace * Tweak comments * Create Merge-JsonOutput function * Undo pds1 file modification * Correct config bug for OutJsonFileName * Mock Merge-JsonOutput function in the RunCached test * Modify pester tests * Update PowerShell/ScubaGear/Testing/Unit/PowerShell/Orchestrator/Invoke-RunCached.Tests.ps1 Co-authored-by: Richard Crutchfield <crutchfield@users.noreply.github.com> * Add switch for MergeJson * Update README.md with MergeJson * Change keys to "Tool" and "ToolVersion" Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com> --------- Co-authored-by: Richard Crutchfield <crutchfield@users.noreply.github.com> Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com>
* Update regal version * debug * import rego.v1 * remove if {} * replace boolean asignments * Up regal setup version * Fix review issues * Add default OPA version
I have updated this to utilize a variable rather than calling the same cmdlet multiple times and grouped the code together now. Please take a look at it when you get a chance. |
Updated. |
@dagarwal-mitre @nanda-katikaneni FYI - I'd like to ensure that this PR works on the G3 tenant before merging. In particular we need to ensure that the license message for policies not available in G3 tenants shows up in the report since we have changed the foundational code for handling the G3 license. Also I'd like to run the functional test orchestrator against the G3. |
…b.com/cisagov/ScubaGear into 778-add-licensing-information-to-html
Testing updateTested the most recent code updates where Devesh grouped processing of the licenses into the same code block against the G5, GCC High and E5 tenants. The code did not break any existing functionality that relies on the AAD license info. Once the G3 tenant is backup up we need to test there to ensure that the licensing report works with G3 and do a regression to ensure the existing Rego policies that check for the AAD premium license still work since the related provider code was modified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Results of the current PR are consistent with the direction the group has decided on for licensing information. All of my comments have been addressed and the TODO issue has been filed. Looks good to me.
Setting status to blocked until the G3 test tenant expired license is resolved so this PR can be tested against that level prior to merge. |
Work here moved to new PR #1011 to better handle branch conflicts. Closing. |
🗣 Description
This PR concludes adding licensing information to ScubaGear.
💭 Motivation and context
The licensing information has been added to the bottom part of the AAD report for users to be able to review. This will conclude the larger epic for adding licensing information to ScubaGear (#466)
Closes #778
🧪 Testing
This was tested using the different tenants and checking to make sure that the report will still generate without issues even if the AAD report isn't run.
✅ Pre-approval checklist
✅ Pre-merge checklist
PR passed smoke test check.
Feature branch has been rebased against changes from parent branch, as needed
Use
Rebase branch
button below or use this reference to rebase from the command line.Resolved all merge conflicts on branch
Notified merge coordinator that PR is ready for merge via comment mention
✅ Post-merge checklist