search

PowerShell / SecretManagement

PowerShell module to consistent usage of secrets through different extension vaults
MIT License
317 stars 46 forks source link

Sort the results of `Get-SecretInfo` correctly #219

Open andyleejordan opened 4 months ago

andyleejordan commented 4 months ago

Instead of using a SortedDictionary which introduced a bug because it required the keys (secret names) to be unique, we just sort the array directly using a comparer that sorts by name. Fixes #95.

JustinGrote commented 4 months ago

@andyleejordan are there any potential regressions with, say, piping two secretinfo results with the same name to get-secret?

andyleejordan commented 4 months ago

I am unsure and need to get some more folks to look at this. Also, I had a thought that it probably doesn't even need to build a set, I don't see why we can't just sort the array with the comparer I added.

andyleejordan commented 4 months ago

Yup, that's better. No idea why this had a dictionary in the first place, all it needed was a comparer.

andyleejordan commented 4 months ago

@andyleejordan are there any potential regressions with, say, piping two secretinfo results with the same name to get-secret?

Well, instead of erroring because of the duplicate names it should now correctly print them both.

SteveL-MSFT commented 4 months ago

It's too bad we didn't catch this sooner and make the secret name case-insensitive, but it would be a breaking change now.

andyleejordan commented 4 months ago

Fair 😆 I'll figure that out...I need to make another PR setting up build/test tasks because that's been the slowest part of this.

andyleejordan commented 4 months ago

It's too bad we didn't catch this sooner and make the secret name case-insensitive, but it would be a breaking change now.

My understanding of it is that secret names aren't necessarily case-sensitive, at least as far as SecretManagement is concerned. It's up to the vault implementations to decide their case sensitivity...which is what revealed this bug. I think SecretStore is case-sensitive, but that shouldn't really matter here.