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

Sort the results of Get-SecretInfo correctly #219

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andyleejordan
Copy link
Member

@andyleejordan andyleejordan commented Feb 16, 2024

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
Copy link

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

@andyleejordan
Copy link
Member Author

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 andyleejordan changed the title Use a SortedSet to sort the results of Get-SecretInfo Sort the results of Get-SecretInfo correctly Feb 20, 2024
@andyleejordan
Copy link
Member Author

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

@andyleejordan
Copy link
Member Author

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

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

You should add a test.

@SteveL-MSFT
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Get-SecretInfo: An item with the same key has already been added" exception should be nonterminating
3 participants