feat/secretstore

[pravinpushkar]

Description

Please explain the changes you've made

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #43

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation

[philliphoff]

IReadOnlyDictionary<string, IReadonlyDictionary<string, string>> would be the most raw way to expose the bulk data, but I'm not sure if that'd be the best way, as it doesn't easily allow for changes to the API in the future. (E.g. suppose the bulk request returns both values and other metadata for each individual secret). It'd probably be better to expose something like IReadOnlyDictionary<string, GetSecretResponse> Data (which is actually closer to the .proto definition anyway).

I think it's fine to try to simplify some of the .proto APIs where it makes sense (e.g. where it contains unnecessary layers), but we also want to walk that fine line of being true to the .proto as well so that the .NET APIs are still conceptually similar to the other SDKs.

[pravinpushkar]

I think we can use the SecretResponse() constructor from the proto defintion.

[philliphoff]

What's the Dapr semantics for getting a secret key that doesn't exist? Is that an error or should a "does not exist" value be returned? In the case of state stores, we return null in that case.

[pravinpushkar]

in case of env secret store we return empty value like {somsecret: ""}. I think it can depend on specific implementation.

[philliphoff]

Not sure what happened here...

[philliphoff]

I think the XML editor is getting too aggressive about escaping characters; -> is what it should be.

[philliphoff]

Remove the nullability (?) from the response argument? We would always expect the implementation to give us one, right?

[philliphoff]

We'll want to defer committing this PR until (at least) a v1.12.0 tag exists in dapr/dapr, as we don't want builds picking up whatever happens to be in the master branch at the time.

[philliphoff]

C# allows Add() methods to be inlined:

var resp = new Dictionary<string, string>
{
  { request.SecretName, data }
}

[philliphoff]

Nit: var response = new SecretStoreGetResponse allows you to eliminate the nullable response created above.

[philliphoff]

Nit: should be string? data = ...?

I'd expect a build warning about nullability otherwise (as GetEnvironmentVariable() returns string?).

[pravinpushkar]

GetEnvironmentVariable returns string when environment variable is not found or set.

[philliphoff]

GetEnvironmentVariable() returns string? (a potentially null string). An alternative would be:

string data = Environment.GetEnvironmentVariable() ?? String.Empty;

...which assigns "" only if the return value is null.

[philliphoff]

GetEnvironmentVariables() returns a dictionary of both names and values; there shouldn't be a reason to later call GetEnvironmentVariable() for each variable name.

[philliphoff]

It might be good to add <remarks> that describe the expected behavior for secrets that don't exist as well as differences between stores that support multiple values per secret and those that don't.

[philliphoff]

Generally on the right track. There's a question for how to represent bulk secret responses, and it'd be helpful to include unit tests for the new components and data types. (I just committed a change that switches tests to NSubstitute from Moq, so be sure to merge in the main branch.)

[philliphoff]

This should only happen once, right, outside the foreach and for the item.Key key?

[philliphoff]

response should be non-null (by nullability contract). While the implementor could still technically return null at runtime, I'd say that's their problem and not something to explicitly account for.

[philliphoff]

response should be non-null (by nullability contract). While the implementor could still technically return null at runtime, I'd say that's their problem and not something to explicitly account for.

[philliphoff]

@pravinpushkar With 1.12 now out the door, we should be able to pick this PR back up, right?

[pravinpushkar]

@pravinpushkar With 1.12 now out the door, we should be able to pick this PR back up, right?

@philliphoff Yes, I will plan to complete this in coming weeks.

[WhitWaldo]

Several changes. Mostly around naming consistency questions, a potentially unnecessary type and some assignment/nullability nits.

[WhitWaldo]

@philliphoff Good to merge?