feat: Support credential type parameter for NLog #15270
Conversation
robertvoinescu-work
changed the title
robertvoinescu-work
force-pushed
the
nlog/windy-eagle
branch
from
764abb3 to
d42dcc4
Compare
apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog.csproj
Outdated
Show resolved
Hide resolved
| This should typically be pulled in as a transitive dependency of GAX, but for now | ||
| GAX doesn't reference the latest version. | ||
| --> | ||
| <PackageReference Include="Google.Apis.Auth" VersionOverride="1.73.0" /> |
There was a problem hiding this comment.
TODO: Remove before merge, we will rely on the gax transitive dependency.
There was a problem hiding this comment.
Will just mark this a change request so that we can remember.
There was a problem hiding this comment.
Generated the new project files. Resolving.
amanda-tarafa
added
the
do not merge
amanda-tarafa
added
do not merge
| This should typically be pulled in as a transitive dependency of GAX, but for now | ||
| GAX doesn't reference the latest version. | ||
| --> | ||
| <PackageReference Include="Google.Apis.Auth" VersionOverride="1.73.0" /> |
There was a problem hiding this comment.
Will just mark this a change request so that we can remember.
apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog.Tests/GoogleStackdriverTargetTest.cs
Outdated
Show resolved
Hide resolved
...Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/GoogleStackdriverTarget_Configuration.cs
Outdated
Show resolved
Hide resolved
| /// Valid strings can be found in the <see cref="JsonCredentialParameters"/> class. | ||
| /// Defaults to <see cref="JsonCredentialParameters.ServiceAccountCredentialType"/>. | ||
| /// </summary> | ||
| public Layout CredentialType { get; set; } = JsonCredentialParameters.ServiceAccountCredentialType; |
There was a problem hiding this comment.
What happens if this is set to null? Where do we fail?
There was a problem hiding this comment.
This fails at the CredentialFactory in the Google.Apis.Auth layer.
| var nullLogEvent = LogEventInfo.CreateNullEvent(); | ||
| var credentialFile = CredentialFile?.Render(nullLogEvent); | ||
| var credentialJson = CredentialJson?.Render(nullLogEvent); | ||
| var credentialType = CredentialType?.Render(nullLogEvent); |
There was a problem hiding this comment.
How does the default work? Can this be null even though we have a default, and what happens if it is null?
There was a problem hiding this comment.
If no value is specified for the key or the key value pair is omitted then the default will be used, but if it is set to an explicit null or the empty string "" then that will be passed forward to the CredentialFactory which will fail.
robertvoinescu-work
force-pushed
the
nlog/windy-eagle
branch
from
d42dcc4 to
908f8fb
Compare
| /// Because ADC resolution depends on the 'GOOGLE_APPLICATION_CREDENTIALS' environment | ||
| /// variable—which cannot be securely modified in our test environment. | ||
| /// </remarks> | ||
| public class CredentialTests |
There was a problem hiding this comment.
I've removed the corresponding integration tests which validate these cases and moved theme here. On top of this I took the opportunity to test things from the level a config xml is specified and added a couple more test cases.
2 participants
No description provided.