- Notifications
You must be signed in to change notification settings - Fork676
Add support for query ldap groups links#1612
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
For issue#1609 |
Have tested this manually on a real gitlab instance. I could not see any automated tests for gitlab -> ldap features |
codecov-commenter commentedSep 26, 2021 • edited by codecovbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by codecovbot
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #1612 +/- ##==========================================+ Coverage 91.59% 91.61% +0.01%========================================== Files 74 74 Lines 4272 4279 +7 ==========================================+ Hits 3913 3920 +7 Misses 359 359
Flags with carried forward coverage won't be shown.Click here to find out more.
🚀 New features to boost your workflow:
|
Fixed 2 linting issues. One was commit message so had to force push |
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 a lot@andrewlarssen! Just a comment on naming consistency. If you're able to add some unit tests to this, would also be great! Herehttps://github.com/python-gitlab/python-gitlab/blob/master/tests/unit/objects/test_groups.py or you could create a newtest_ldap_links.py if it's getting too crowded there. Unit tests mock the responses so we can test EE features there.
While reviewing this I noticed our existing method for deletion uses deprecated API endpoints:https://docs.gitlab.com/ee/api/groups.html#delete-ldap-group-link
So as a bonus if you find time, I think these methods actually could be removed and you can add the CreateMixin/DeleteMixin to the manager, and ObjectDeleteMixin to the object. Then we could remove the custom methods here:
python-gitlab/gitlab/v4/objects/groups.py
Lines 112 to 150 in227607c
| @cli.register_custom_action("Group", ("cn","group_access","provider")) | |
| @exc.on_http_error(exc.GitlabCreateError) | |
| defadd_ldap_group_link(self,cn,group_access,provider,**kwargs): | |
| """Add an LDAP group link. | |
| Args: | |
| cn (str): CN of the LDAP group | |
| group_access (int): Minimum access level for members of the LDAP | |
| group | |
| provider (str): LDAP provider for the LDAP group | |
| **kwargs: Extra options to send to the server (e.g. sudo) | |
| Raises: | |
| GitlabAuthenticationError: If authentication is not correct | |
| GitlabCreateError: If the server cannot perform the request | |
| """ | |
| path="/groups/%s/ldap_group_links"%self.get_id() | |
| data= {"cn":cn,"group_access":group_access,"provider":provider} | |
| self.manager.gitlab.http_post(path,post_data=data,**kwargs) | |
| @cli.register_custom_action("Group", ("cn",), ("provider",)) | |
| @exc.on_http_error(exc.GitlabDeleteError) | |
| defdelete_ldap_group_link(self,cn,provider=None,**kwargs): | |
| """Delete an LDAP group link. | |
| Args: | |
| cn (str): CN of the LDAP group | |
| provider (str): LDAP provider for the LDAP group | |
| **kwargs: Extra options to send to the server (e.g. sudo) | |
| Raises: | |
| GitlabAuthenticationError: If authentication is not correct | |
| GitlabDeleteError: If the server cannot perform the request | |
| """ | |
| path="/groups/%s/ldap_group_links"%self.get_id() | |
| ifproviderisnotNone: | |
| path+="/%s"%provider | |
| path+="/%s"%cn | |
| self.manager.gitlab.http_delete(path) |
This would be a breaking change, but we have a major release coming up anyway. Let me know if this makes sense or you need help. Also, you'll just need to amend your message again as commitlint does not like capitalized sentences :)
| "GroupLdapLinks", | ||
| "GroupLdapLinksManager", |
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.
For consistency with other objects, we should use the single (the API also supports adding a single link)
| "GroupLdapLinks", | |
| "GroupLdapLinksManager", | |
| "GroupLdapLink", | |
| "GroupLdapLinkManager", |
| subgroups:"GroupSubgroupManager" | ||
| variables:GroupVariableManager | ||
| wikis:GroupWikiManager | ||
| ldap_group_links:"GroupLdapLinksManager" |
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.
| ldap_group_links:"GroupLdapLinksManager" | |
| ldap_group_links:"GroupLdapLinkManager" |
| _types= {"skip_groups":types.ListAttribute} | ||
| classGroupLdapLinks(RESTObject): |
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.
| classGroupLdapLinks(RESTObject): | |
| classGroupLdapLink(RESTObject): |
| classGroupLdapLinksManager(ListMixin,RESTManager): | ||
| _path="/groups/%(group_id)s/ldap_group_links" | ||
| _obj_cls=GroupLdapLinks |
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.
| classGroupLdapLinksManager(ListMixin,RESTManager): | |
| _path="/groups/%(group_id)s/ldap_group_links" | |
| _obj_cls=GroupLdapLinks | |
| classGroupLdapLinkManager(ListMixin,RESTManager): | |
| _path="/groups/%(group_id)s/ldap_group_links" | |
| _obj_cls=GroupLdapLink |
This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days. |
This PR was closed because it has been marked stale for 15 days with no activity. If this PR is still valid, please re-open. |
You can currently add or delete ldap links but you can't query existing links