-
Notifications
You must be signed in to change notification settings - Fork 183
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 Scan
functionality for assets-maintenance-tool
#6269
Conversation
… be used for integration tests
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run tools - assets-maintenance-tool - tests |
Commenter does not have sufficient privileges for PR 6269 in repo Azure/azure-sdk-tools |
/azp run tools - assets-maintenance-tool - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
...-automation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool.Tests/CLITests.cs
Show resolved
Hide resolved
.../assets-automation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Program.cs
Outdated
Show resolved
Hide resolved
...omation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Scan/AssetsScanner.cs
Outdated
Show resolved
Hide resolved
...omation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Scan/AssetsScanner.cs
Outdated
Show resolved
Hide resolved
...omation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Scan/AssetsScanner.cs
Outdated
Show resolved
Hide resolved
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.
I left bunch of nitpicky comments. I commented more than I usually do, because this is the first time I am looking into your stuff in details, so I stated bunch of my preferences and looking forward to hearing what's your take. Overall, I really appreciate the nice domain model and small methods!
...e.Sdk.Tools.Assets.MaintenanceTool.Tests/Azure.Sdk.Tools.Assets.MaintenanceTool.Tests.csproj
Show resolved
Hide resolved
...-automation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool.Tests/CLITests.cs
Show resolved
Hide resolved
...omation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Scan/AssetsScanner.cs
Outdated
Show resolved
Hide resolved
...omation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Scan/AssetsScanner.cs
Outdated
Show resolved
Hide resolved
...omation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Scan/AssetsScanner.cs
Outdated
Show resolved
Hide resolved
...omation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Scan/AssetsScanner.cs
Outdated
Show resolved
Hide resolved
...omation/assets-maintenance-tool/Azure.Sdk.Tools.Assets.MaintenanceTool/Scan/AssetsScanner.cs
Outdated
Show resolved
Hide resolved
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.
I left a bunch of nitpicky comments. I commented more than I usually do, because this is the first time I am looking into your stuff in detail. I stated bunch of my preferences and looking forward to hearing what's your take. Overall, I really appreciate the nice domain model and small, easy to follow methods!
Co-authored-by: Konrad Jamrozik <[email protected]>
…s.Assets.MaintenanceTool/Scan/AssetsScanner.cs Co-authored-by: Konrad Jamrozik <[email protected]>
…s.Assets.MaintenanceTool/Scan/AssetsScanner.cs Co-authored-by: Konrad Jamrozik <[email protected]>
Nice thank you for the detailed review! I've address the vast majority of them already, and am doing a slightly deeper refactor on the using suggestion + disk io comment. Additionally I took the feedback about |
Awesome, glad to hear my feedback was useful!! Can you ping me when I should have another look? (which is optional; feel free to merge without it, too) |
/azp run tools - assets-maintenance-tool - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run tools - assets-maintenance-tool - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
Merged. I took most everything into account except for FileIO updates. |
* Added switch to skip sync and generate script call * Support forked spec repo in regex * Rebase from main for below changes: Asset-Sync Documentation Update (#6303) add line offering additional detail remove extraneous newline Add `Scan` functionality for assets-maintenance-tool (#6269) Add new project `assets-maintenance-tool`. Implement `scan` functionality within. `backup` and `cleanup` still to come. Add integration tests for new assets-maintenance tool. Move original `asset-sync` powershell proto script into `tools/assets-automation/` folder Move `eng/scripts/python/assets-automation/` under `tools/assets-automation/assets-reporting/` Add ci.yml and tests.yml to invoke integration tests against the azure-sdk-assets-integration Co-authored-by: Konrad Jamrozik <[email protected]> Co-authored-by: Scott Beddall <[email protected]> Co-authored-by: Konrad Jamrozik <[email protected]> Co-authored-by: Praven Kuttappan <[email protected]>
This PR adds the functionality of
scanning
an input configuration. Part of #4298 . Usesnunit
per @benbp recommendation.Originally, I asked @benbp if I could use SQLLite for the storage medium, but I just went with
json
for now. If this needs to change in the future, I am ready to do so.scan
tool +ci
+livetests
.assets-automation
folder.After this goes in, I'll add the a build definition that invokes this tool and actually starts scanning and backing up our assets graph nightly.