feat(ingest): Add custom properties to the ldap ingestion #7125
Conversation
Upstream master
…into customProperties
github-actions
bot
added
devops
| for prop in self.config.custom_props_list: | ||
| if self.config.user_attrs_map.get(prop) in attrs: | ||
| custom_props_map[prop] = ( | ||
| attrs[self.config.user_attrs_map[prop]][0] |
There was a problem hiding this comment.
This would require each item in custom_props_list to also be included in user_attrs_map. Additionally, user_attrs_map is generally a mapping of datahub name -> custom attr name, not the reverse. I think we should just use the items in custom_props_list directly
There was a problem hiding this comment.
This would require each item in custom_props_list to also be included in user_attrs_map. Additionally, >user_attrs_map is generally a mapping of datahub name -> custom attr name, not the reverse. I think we should just >use the items in custom_props_list directly.
I use "user_attrs_map" to map datahub property to an actual ldap property. For example, I have this mapping in my "user_attrs_map": {"countryCode":"c"}. I do this to replace a default datahub mapping for "countryCode", which is "countryCode":"countryCode", by an actual ldap attribute name for my company, which is "c".
custom_props_list is just a list of my custom datahub properties for corpUser, not an actual ldap attribute's names. For example, in my case I want to use "accountType" as a custom name property. This is why I use "user_attrs_map", I need to map "accountType" to an actual ldap attribute.
I think instead you suggested to use a ldap attribute name directly at custom_props_list and not to use an alias. This could be done, but I believe we are going to have some logic in UI build on values of some custom properties, not just display them. In this case, if ldap attribute name is changed, we will need to rename all place in UI code with a hardcoded previous ldap attribute name and we want to avoid this by using aliases.
However, I am going to check with my team, I think we can simplify this and use ldap attribute name directly.
There was a problem hiding this comment.
@hsheth2, I am going to make an update you have suggested.
|
Thanks for updating @bda618! We are taking another pass tomorrow. Cheers |
| @@ -117,6 +117,10 @@ class LDAPSourceConfig(StatefulIngestionConfigBase): | |||
| default=None, description="Retrieved attributes list" | |||
| ) | |||
|
|
|||
| custom_props_list: Optional[List[str]] = Field( | |||
| default=None, description="Customer attributes list" | |||
There was a problem hiding this comment.
| default=None, description="Customer attributes list" | |
| default=None, description="A list of custom attributes to extract from the LDAP provider." |
There was a problem hiding this comment.
@hsheth2 Yes, your description is a way more accurate, will update today.
There was a problem hiding this comment.
@hsheth2, will update ldap test too.
…oject#7125) Co-authored-by: Harshal Sheth <[email protected]>
…oject#7125) Co-authored-by: Harshal Sheth <[email protected]>
Checklist