ScubaGear icon indicating copy to clipboard operation
ScubaGear copied to clipboard

Improve Cached Tenant Settings cmdlet support

Open schrolla opened this issue 2 years ago • 5 comments

These are my comments on Run-Cached:

  • I tested it and no errors. It only works if there is a ProviderSettingsExport.json in the current directory. See my last bullet.

  • [x] Rename it to make it unique just like we did for Disconnect-Tenant. FYI - Cassey is referencing Run-Cached in her regression test script. (This may be OBE as regression test script likely replaced by new testing framework.) This is now done and was renamed Invoke-ScubaCached to differentiate it and make it more unique

  • [ ] When I execute Run-Cached without any parameters, it simply acts like Invoke-Scuba. I think we should change the default behavior to distinguish it. Perhaps by default it will set the -ExportProvider $false ?

  • [x] Since we are exporting Invoke-ScubaCached, do we need to add any instructions to the README? Or do we simply let developers figure this script out by examining the code on their own? This is now documented in the functional testing README.

  • [ ] I think the script should output a friendly message when there is no ProviderSettingsExport.json file in the current directory or in OutPath. Right now the user gets a bunch of generic errors.

_Originally posted by @tkol2022

  • [x] Should also be renamed from Invoke-RunCached to Invoke-SCuBACached

schrolla avatar Dec 22 '22 14:12 schrolla

There are a number of improvements included here. These should be separately refined and added as separate issues with this item as the overall epic to address.

schrolla avatar Nov 22 '23 17:11 schrolla

@crutchfield Can you take a look and see if there's much need here beyond wanting to fix up naming or if the other items are still relevant?

schrolla avatar Dec 15 '23 19:12 schrolla

Is it worth some way of indicating the output was by Invoke-RunCached instead of Invoke-Scuba?

Looking at the items listed, I recommend the following:

  1. Rename it to make it unique just like we did for Disconnect-Tenant. FYI - Cassey is referencing Run-Cached in her regression test script. (This may be OBE as regression test script likely replaced by new testing framework.) I agree rename Invoke-RunCached to Invoke-ScubaRunCached is value added..
  2. When I execute Run-Cached without any parameters, it simply acts like Run-Scuba. I think we should change the default behavior to distinguish it. Perhaps by default it will set the -ExportProvider $false ? Good idea. In the functional tests we used ModifiedProviderSettings.json.
  3. Since we are exporting Run-Cached, do we need to add any instructions to the README? Or do we simply let developers figure this script out by examining the code on their own? I would not expect the average user to use this function. So I'd recommend we document in a deve;oper/contributor area instead of main user documentation. A nefarious user could use to fudge results to report. Otherwise, I would have suggested we just add another (already to many) switch to Invoke-Scuba for calling Invoke-RunCached and not export it.
  4. I think the script should output a friendly message when there is no ProviderSettingsExport.json file in the current directory or in OutPath. Right now the user gets a bunch of generic errors. Agree

crutchfield avatar Dec 18 '23 13:12 crutchfield

Break this into small separate issues for easy resolution.

schrolla avatar Jan 19 '24 19:01 schrolla

I'm removing myself from this for now because it is crowding my to-do list and this is very low priority compared to the plethora of other items we have in our buckets.

tkol2022 avatar Jul 25 '24 16:07 tkol2022