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
CLOUDP-72727: Fix LDAPConfigurationsService.Save to pass LDAPConfiguration as input #136
Conversation
It's always good to describe the bug and fix in the description - it will help in the future if we look at this PR. |
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, so easy to make those sort of mistakes :)
[q] Would the e2e tests catch an issue like this? It seems really easy to do and wondering how we can prevent in the future.
@@ -133,7 +133,7 @@ func TestLDAPConfigurations_Save(t *testing.T) { | |||
}`) | |||
}) | |||
|
|||
request := &LDAP{} | |||
request := &LDAPConfiguration{} |
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.
yes, I know. I am not sure how to prevent this issue though 😕 maybe we can populate the struct that we pass to function as well.
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.
Yeah, maybe in the test for the call, then at least you purposefully expect certain values to be present in the input. Not blocking this PR, but something to consider.
@colm-quinn ready for review |
request := &LDAPConfiguration{ | ||
LDAP: &LDAP{ | ||
AuthenticationEnabled: true, | ||
AuthorizationEnabled: true, | ||
Hostname: "atlas-ldaps-01.ldap.myteam.com", | ||
Port: 636, | ||
BindUsername: "CN=Administrator,CN=Users,DC=atlas-ldaps-01,DC=myteam,DC=com", | ||
BindPassword: "admin", | ||
}, |
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.
Nice!
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, thanks for the changes
@themantissa we'll merge this to not block the implementation of mongocli but feel free to tag DoU for a post merge review if you folks will use this for terraform |
Left a note in Jira for when DoU starts LDAP work INTMDB-13 |
Description
I discovered bugs in the implementation of the LDAP configuration api call #135 (review) 346af62:
LDAPConfigurationsService.Save
was using the structLDAP
instead ofLDAPConfiguration
LDAPConfigurationsService.Delete
was not correctType of change:
Required Checklist:
make fmt
and formatted my codeFurther comments