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

Make ScubaResults.json the default JSON output #1316

Merged
merged 16 commits into from
Sep 20, 2024

Conversation

buidav
Copy link
Collaborator

@buidav buidav commented Sep 16, 2024

🗣 Description

  • Makes ScubaResults.json the default output for ScubaGear
  • Inverts the -MergeJSON switch to -KeepIndividualJSON parameter.
  • Make ScubaCached backwards compatible with ScubaResults.json
  • Update any tests that need it to match the new ScubaGear output behavior.

TODO:

💭 Motivation and context

Closes #1205
Closes #1023

🧪 Testing

  • Run Invoke-SCuBA. Observe ScubaResults.json is the default output.

  • Run Invoke-SCuBA -KeepIndividualJSON. Observe ProviderSettingsExport.json is output instead of ScubaResutls.

  • Run Invoke-SCuBACached. Observe ScubaResults.json is the default output.

  • Run Invoke-SCuBACached -ExportProvider $false. Observe ScubaCached still performs as expected.

  • Run Invoke-SCuBACached with the -KeepIndividualJSON flag. Observe performance benefit with this flag. Along with no change in behavior.

  • Run functional tests for a product as see no breaking changes.

  • For reviewers and merge coordinator Check smoke tests or other workflows to see if there are errors from the new output behavior.

✅ 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.

@buidav buidav added breaking change This issue or pull request involves changes to existing functionality enhancement This issue or pull request will add new or improve existing functionality labels Sep 16, 2024
@buidav buidav added this to the Jellyfish milestone Sep 16, 2024
@buidav buidav linked an issue Sep 16, 2024 that may be closed by this pull request
@buidav buidav marked this pull request as ready for review September 16, 2024 23:38
@buidav buidav force-pushed the 1205-make-mergejson-the-default branch from 34005dc to e7198dd Compare September 16, 2024 23:47
Copy link
Collaborator

@adhilto adhilto left a comment

Choose a reason for hiding this comment

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

Some minor things:

docs/configuration/parameters.md Outdated Show resolved Hide resolved
docs/configuration/parameters.md Show resolved Hide resolved
@buidav buidav requested a review from adhilto September 17, 2024 03:49
Copy link
Collaborator

@mitchelbaker-cisa mitchelbaker-cisa left a comment

Choose a reason for hiding this comment

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

Ran all of these successfully with no issue. A few small comments to add is all.

  • Run Invoke-SCuBA. Observe ScubaResults.json is the default output.
  • Run Invoke-SCuBA -KeepIndividualJSON. Observe ProviderSettingsExport.json is output instead of ScubaResutls.
  • Run Invoke-SCuBACached. Observe ScubaResults.json is the default output.
  • Run Invoke-SCuBACached -ExportProvider $false. Observe ScubaCached still performs as expected.
  • Run Invoke-SCuBACached with the -KeepIndividualJSON flag. Observe performance benefit with this flag. Along with no change in behavior.
  • Run functional tests for a product as see no breaking changes (ran aad functional tests successfully)

Copy link
Collaborator

@adhilto adhilto left a comment

Choose a reason for hiding this comment

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

Looks great. A few final notes:

@buidav buidav force-pushed the 1205-make-mergejson-the-default branch from 105294d to 4b94871 Compare September 19, 2024 16:26
@buidav
Copy link
Collaborator Author

buidav commented Sep 19, 2024

@nanda-katikaneni this is ready to merge.

Not sure why the checks are saying unsuccessful when looking at them individually they're all passing.
Fixed it.

@nanda-katikaneni nanda-katikaneni merged commit b8f5480 into main Sep 20, 2024
13 checks passed
@nanda-katikaneni nanda-katikaneni deleted the 1205-make-mergejson-the-default branch September 20, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This issue or pull request involves changes to existing functionality enhancement This issue or pull request will add new or improve existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make MergeJson the default behavior Make Invoke-ScubaCached backwards compatible with ScubaResults.json
5 participants