Skip to content
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

feature/mx-1455-document-ldap-extractor #33

Merged
merged 10 commits into from
Oct 20, 2023

Conversation

rababerladuseladim
Copy link
Contributor

@rababerladuseladim rababerladuseladim commented Oct 13, 2023

This adds general documentation for the ldap extractor.

While reviewing the tests I noticed that they cover none of the convenience functions (ldap.extract). Should I create a ticket for that?

mr-kamran-ali
mr-kamran-ali previously approved these changes Oct 20, 2023
Copy link
Contributor

@mr-kamran-ali mr-kamran-ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clear to me.

@mr-kamran-ali
Copy link
Contributor

I think there should be tests for ldap.extract convenience functions. yes, please create a ticket.

@rababerladuseladim
Copy link
Contributor Author

I think there should be tests for ldap.extract convenience functions. yes, please create a ticket.

See https://jira.rki.local/browse/MX-1477

# Conflicts:
#	mex/common/public_api/connector.py
@rababerladuseladim rababerladuseladim merged commit 7114ab7 into main Oct 20, 2023
4 checks passed
@rababerladuseladim rababerladuseladim deleted the feature/mx-1455-document-ldap-extractor branch October 20, 2023 13:26
Copy link
Contributor

@erichesse erichesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mex/common/ldap/README.md::line28ff

I'm irritated by the fact, that ldap.extract is described as a module for convenience functions only. the naming implies that the extract part of the ETL pipeline happens here. But actually that happens in ldap.connect.

if ldap.extract is for helper functions to build a mapping, maybe it should be rename to map.py or something. And ldap.connector should be renamed to ldap.extract.

I am open to other solutions, but right now, I find the structure to be misleading.

Otherwise I like the README, good work

@rababerladuseladim

@rababerladuseladim
Copy link
Contributor Author

That is indeed a point. I noticed that, too while writing the docs but did not know how to address this. Connectors are a common theme, so I'd rather leave the connector module untouched. Regarding the helper functions: In the organigram extractor these also reside in extract.py, hence I'd like to leave that in sync, too. One idea might be to add a new utils.py module and move any helper functionality in there. What do you think?

@erichesse
Copy link
Contributor

erichesse commented Oct 23, 2023

yes, sounds good to me

@rababerladuseladim
Copy link
Contributor Author

I created a ticket: https://jira.rki.local/browse/MX-1479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants