-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add support for true/false string literals for agent injector #22996
Conversation
Build Results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Would you be able to throw in some go doc comments for the test methods please?
Side note, what do you think about a table driven test for the authenticate_from_environment
testing?
func TestAzureAuthMethod_AuthenticateFromEnvironment(t *testing.T) {
t.Parallel()
tests := map[string]struct {
AuthenticateFromEnvValue any
IsErrorExpected bool
}{
"bool-true": {
AuthenticateFromEnvValue: true,
IsErrorExpected: false,
},
"bool-false": {
AuthenticateFromEnvValue: false,
IsErrorExpected: false,
},
"string-true": {
AuthenticateFromEnvValue: "true",
IsErrorExpected: false,
},
"string-false": {
AuthenticateFromEnvValue: "false",
IsErrorExpected: false,
},
"string-empty": {
AuthenticateFromEnvValue: "",
IsErrorExpected: false,
},
"nil": {
AuthenticateFromEnvValue: nil,
IsErrorExpected: false,
},
"string-bad": {
AuthenticateFromEnvValue: "bad_value",
IsErrorExpected: true,
},
}
for name, tc := range tests {
name := name
tc := tc
t.Run(name, func(t *testing.T) {
t.Parallel()
config := &auth.AuthConfig{
Logger: hclog.NewNullLogger(),
MountPath: "auth-test",
Config: map[string]interface{}{
"resource": "test",
"client_id": "test",
"role": "test",
"scope": "test",
"authenticate_from_environment": tc.AuthenticateFromEnvValue,
},
}
_, err := NewAzureAuthMethod(config)
if tc.IsErrorExpected {
require.Error(t, err)
} else {
require.NoError(t, err)
}
})
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Added the godocs.
I personally find that table driven tests feel a little overkill for tests like this -- we're not saving a lot of complexity/repetition really, and as-is, tests are nice and simple and self-contained, with a lot less to think about when reading the test. I think it's a preference thing, but that's why I did it this way.
I did add the t.Parallels though! Almost forgot those :)
CI Results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
The Agent Injector doesn't support boolean literals, so the ability for us to convert string to bool makes things a lot easier to use.
The 1.15.1 milestone doesn't exist, but if it did, I'd put it on this PR.