-
Notifications
You must be signed in to change notification settings - Fork 34
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
INTMDB-301: Feature add: Add support for management of federated authentication configuration #296
Conversation
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.
Thanks for adding these resources. I have some minor comments
return nil, NewArgError("federationSettingsID", "must be set") | ||
} | ||
|
||
basePath := fmt.Sprintf(federationSettingsOrganizationConnectionBasePath, federationSettingsID, orgID) |
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.
same as above
@@ -0,0 +1,157 @@ | |||
// Copyright 2019 MongoDB Inc |
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.
// Copyright 2019 MongoDB Inc | |
// Copyright 2022 MongoDB Inc |
@@ -0,0 +1,76 @@ | |||
// Copyright 2019 MongoDB Inc |
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.
// Copyright 2019 MongoDB Inc | |
// Copyright 2022 MongoDB Inc |
@gssbzn is there anyone else on the team that can review this while Andrea is out on vacation? |
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.
There's a big change required here which is orgnaizing all the added method under the same service to follow our practice of grouping the same way the docs group
mongodbatlas/federation_settings.go
Outdated
Get(context.Context, string) (*FederatedSettings, *Response, error) | ||
Delete(context.Context, string) (*Response, error) | ||
ListConnectedOrgs(context.Context, *ListOptions, string) (*FederatedSettingsConnectedOrganizations, *Response, error) | ||
GetConnectedOrgs(context.Context, string, string) (*FederatedSettingsConnectedOrganization, *Response, error) |
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.
GetConnectedOrgs(context.Context, string, string) (*FederatedSettingsConnectedOrganization, *Response, error) | |
GetConnectedOrg(context.Context, string, string) (*FederatedSettingsConnectedOrganization, *Response, error) |
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.
Some small nits, I still see many of andreas comments unaddressed, common themes, godocs need to be reviewed, and let's keep list options as last params
mongodbatlas/federation_settings.go
Outdated
UpdateRoleMapping(context.Context, string, string, string, *FederatedSettingsOrganizationRoleMapping) (*FederatedSettingsOrganizationRoleMapping, *Response, error) | ||
DeleteRoleMapping(context.Context, string, string, string) (*Response, error) | ||
|
||
ListIdentityProvider(context.Context, *ListOptions, string) ([]FederatedSettingsIdentityProvider, *Response, error) |
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.
ListIdentityProvider(context.Context, *ListOptions, string) ([]FederatedSettingsIdentityProvider, *Response, error) | |
ListIdentityProviders(context.Context, *ListOptions, string) ([]FederatedSettingsIdentityProvider, *Response, error) |
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.
Refactored to ListIdentityProviders(context.Context, string, *ListOptions) ([]FederatedSettingsIdentityProvider, *Response, error)
Co-authored-by: Gustavo Bazan <[email protected]>
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 all the work here
Description
Add support for managment of federated authentication configuration
Link to any related issue(s):
https://jira.mongodb.org/browse/INTMDB-301
Type of change:
Required Checklist:
make fmt
and formatted my codeFurther comments