Parse Json param with fallback to default way of parsing i.e. string or stringList #181
Conversation
|
|
||
| namespace Amazon.Extensions.Configuration.SystemsManager | ||
| { | ||
| public class JsonOrDefaultParameterProcessor : DefaultParameterProcessor |
There was a problem hiding this comment.
We need docs here explaining what this processor does. I would suggest instead of using the term Default here I would use String. So it is obvious to users that this supports either Json or falls back to String if the parameter fails to parse as Json.
There was a problem hiding this comment.
Yeah I was thinking that, but then I hold myself cause it's both String and StringList
If you think JsonOrStringParameterProcessor would be good enough I can change that.
There was a problem hiding this comment.
It is a valid concern that it is also doing StringList but I feel like String is better then Default. If I read the name JsonOrDefault.. and ignore my knowledge of how things are implemented I would see a name that says Json or some unknown format.
There was a problem hiding this comment.
Yeah used JsonOrStringParameterProcessor
There was a problem hiding this comment.
added docs in places where I felt, open for suggestion
| { | ||
| foreach (var kv in JsonConfigurationParser.Parse(parameter.Value)) | ||
| { | ||
| var key = !string.IsNullOrEmpty(keyPrefix) ? ConfigurationPath.Combine(keyPrefix, kv.Key) : kv.Key; |
There was a problem hiding this comment.
Can you refactor this inner loop logic that is a duplicate inner loop logic in JsonParameterProcessor into a common internal utility method.
There was a problem hiding this comment.
Yeah I was thinking on the same line.
I have 3 static methods -
ParseJson can be used in both the new class and json processor.
Other 2 ParseString and ParseStringList can also be extracted out and reused.
Let me work on that
There was a problem hiding this comment.
Extracted all 3 methods,
refactored both JsonParameterProcessor and DefaultParameterProcessor
Please review
…ue is being fetched
|
Changes are done from my end. Once reviewed, I'll squash all commits. |
There was a problem hiding this comment.
Checked out the branch, ran the tests myself. Looks good
Passed! - Failed: 0, Passed: 74, Skipped: 0, Total: 74, Duration: 92 ms - Amazon.Extensions.Configuration.SystemsManager.Tests.dll (netcoreapp3.1)
Test run for C:\Dev\Repos\aws-dotnet-extensions-configuration\test\Amazon.Extensions.Configuration.SystemsManager.Tests\bin\Debug\net6.0\Amazon.Extensions.Configuration.SystemsManager.Tests.dll (.NETCoreApp,Version=v6.0)
Microsoft (R) Test Execution Command Line Tool Version 17.10.0 (x64)
Copyright (c) Microsoft Corporation. All rights reserved.
|
@normj @peterrsongg have't heard back. any update on this? |
|
@normj any update on this, when can this be merged? |
Description
Processes a collection of AWS Systems Manager (SSM) Parameters Store parameters, converting them into a dictionary of strings.
This method supports both JSON-formatted parameters as per
JsonParameterProcessor. And plain string & StringList as perDefaultParameterProcessor.The resulting dictionary keys are case-insensitive, as the other 2 implemenation.
Motivation and Context
We have multiple types of SSM gets loaded during application startup.
Mainly Json and Text based. Params are dynamic in nature, so no way to telling if its Json or Text.
We are using this code for more than a week now, so why not merge it in the lib itself.
Testing
mcr.microsoft.com/dotnet/aspnet:8.0-jammynetstandard2.0;netcoreapp3.1as using ARMUnit tests have been added, validated.
No impact on existing area as new implementation of the
IParameterProcessorScreenshots (if appropriate)
Types of changes
Checklist
License