Enhance csv version of output

[adhilto]

🗣 Description

Remove the TestResults.csv file, which was just a csv version of the Rego output and replace it with a new csv which is structured the same way as the Results are in the html files.

💭 Motivation and context

Closes #1026

🧪 Testing

• I tested manually and reviewed output
• I added new PowerShell unit tests

📷 Screenshots (if appropriate)

The old TestResults.csv, which looked like this:

Is replaced with the new ScubaResults.csv file looks like this:

✅ Pre-approval checklist

• This PR has an informative and human-readable title.
• PR targets the correct parent branch (e.g., main or release-name) for merge.
• Changes are limited to a single goal - eschew scope creep!
• Changes are sized such that they do not touch excessive number of files.
• All future TODOs are captured in issues, which are referenced in code comments.
• These code changes follow the ScubaGear content style guide.
• Related issues these changes resolve are linked preferably via closing keywords.
• All relevant type-of-change labels added.
• All relevant project fields are set.
• All relevant repo and/or project documentation updated to reflect these changes.
• Unit tests added/updated to cover PowerShell and Rego changes.
• Functional tests added/updated to cover PowerShell and Rego changes.
• All relevant functional tests passed.
• All automated checks (e.g., linting, static analysis, unit/smoke tests) passed.

✅ 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

• Feature branch deleted after merge to clean up repository.
• Verified that all checks pass on parent branch (e.g., main or release-name) after merge.

[james-garriss]

Hey, @adhilto , did you look at the documentation to see if it needs to be updated IAW your coding changes?

[james-garriss]

@adhilto , I noticed that you created Pester tests files for converting to CSV (ConvertTo-ResultsCsv.Tests.ps1) and for testing plain text formatting (Format-PlainText.Tests.ps1). Breaking out these tests seems like a great idea. It makes me wonder where the corresponding test files are for converting to other file formats and formatting them. I don't see any other files similar to these 2 and I intuit they should exist. If they should exist, but if they are out of scope for your issue, please create one or more other issues to ensure that we eventually get to feature testing parity.

[adhilto]

@adhilto , I noticed that you created Pester tests files for converting to CSV (ConvertTo-ResultsCsv.Tests.ps1) and for testing plain text formatting (Format-PlainText.Tests.ps1). Breaking out these tests seems like a great idea. It makes me wonder where the corresponding test files are for converting to other file formats and formatting them. I don't see any other files similar to these 2 and I intuit they should exist. If they should exist, but if they are out of scope for your issue, please create one or more other issues to ensure that we eventually get to feature testing parity.

We do have test cases for the json (Merge-JsonOutput.tests.ps1). The other output format we have is HTML, which we do have tests for, but you have to look inside the "Create-Report" folder for those. So I think we're good on this front, but let me know if I'm missing anything.

[adhilto]

Hey, @adhilto , did you look at the documentation to see if it needs to be updated IAW your coding changes?

Turns out there is, thanks for the reminder. I'll get that updated soon.

[adhilto]

Hey, @adhilto , did you look at the documentation to see if it needs to be updated IAW your coding changes?

Turns out there is, thanks for the reminder. I'll get that updated soon.

Done

[james-garriss]

In the docs I see this parameter: OutJsonFileName. Should I also see a similar one for CSV?

[adhilto]

In the docs I see this parameter: OutJsonFileName. Should I also see a similar one for CSV?

...yes. Let me try again.

[adhilto]

@james-garriss @buidav Ready for review again

[james-garriss]

Formatting for TestResults.json is odd. Is it easy to do a pretty print before saving the file?

[james-garriss]

TestResults.json needs to be added back the Reports page, as it's a report.

https://github.com/cisagov/ScubaGear/blob/1026-enhance-csv-version-of-output/docs/execution/reports.md

[buidav]

Could you add these 3 columns to the final ScubaResults.csv?
If the formatting looks too off, adding a single space to each row of the new columns makes things clearer.

---

Also, we should highly encourage the -MergeJSON flag then in the README. I would create a TODO issue for it as a placeholder until #1205 is complete.
Adding a note to relvant sections of the documation and adding the -MergeJson flag in our Invoke-SCuBA examples will do the trick.

[james-garriss]

Also, we should highly encourage the -MergeJSON flag then in the README

I still feel like we are missing the right parameter. If someone wants a CSV output, then -MergeJSON is counterintuitive in the extreme. Something like a simple -CSV flag seems better.

[james-garriss]

ScubaResults.json is now missing from the output, and there's the some weird repetitions in the output. (@adhilto , see Slack for details).

[adhilto]

Also, we should highly encourage the -MergeJSON flag then in the README

I still feel like we are missing the right parameter. If someone wants a CSV output, then -MergeJSON is counterintuitive in the extreme. Something like a simple -CSV flag seems better.

Sorry, looks like I didn't explain the changes I made when I converted the PR back to a draft. The CSV output is no longer dependent on the MergeJSON flag. When I initially opened the PR it was, but after your feedback I removed that dependency and the CSV will always be output.

[james-garriss]

Tested results, works great, ty.

[adhilto]

Also, we should highly encourage the -MergeJSON flag then in the README. I would create a TODO issue for it as a placeholder until #1205 is complete.

Done #1286

[adhilto]

@nanda-katikaneni Ready for merge

[nanda-katikaneni]

Merging into the main as the smoke test error is a known issue (related to the current state of E5 tenant and not related to the changes in this pr).