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
Post-sync and Sign Sync Connector #552
Post-sync and Sign Sync Connector #552
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.
Excited about this overall! The main items to me were error handling and documentation, as detailed in the comments.
|
||
if self.version == "v6": | ||
return requests.get(url + "baseUris", headers=self.header()).json()['apiAccessPoint'] + endpoint | ||
return requests.get(url + "base_uris", headers=self.header()).json()['api_access_point'] + endpoint |
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.
We discussed on the call, but error handling in general will greatly improve the usability of the connector. In this instance, the call to access a dictionary key 'api_access_point' fails with a cryptic KeyError if the api key is incorrect.
self.console_org = config['console_org'] if 'console_org' in config else None | ||
self.api_url = self.base_uri() | ||
self.groups = self.get_groups() | ||
self.default_group_id, = [_id for name, _id in self.groups.items() if name == self.DEFAULT_GROUP_NAME] |
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.
Unused field self.default_group_id
|
||
### Config Spec | ||
|
||
| key | type | required? | notes | |
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.
A clearer explanation of what the groups keys mean and how they are related would be helpful. In addition, some discussion of what it means to use multiple sign orgs or umapis and how the sync works in that case.
self.admin_email = config['admin_email'] | ||
self.console_org = config['console_org'] if 'console_org' in config else None | ||
self.api_url = self.base_uri() | ||
self.groups = self.get_groups() |
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.
The question of whether to call the sign API to query groups prior to running the sync tool. Does it make more sense to call this method in the run() call so as to avoid
(1) not breaking the normal sync if the call fails for some reason
(2) a potentially very long delay between when groups are fetched from sign and when they are actually used
I think its good to keep the base_uri() call since it provides immediate feedback if the api key is wrong.
OR - in contrast to the above, does it make sense to add some kind of post sync validation code to check the configuration of a post sync so we know the authentication will be good when the time comes. Again, I'm just thinking in terms of large scale / long sync processes. Might be overkill.
user_sync/post_sync/manager.py
Outdated
""" | ||
for connector in self.connectors: | ||
self.logger.info("Running module " + connector.name) | ||
connector.run(post_sync_data) |
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.
Good opportunity for error handling to avoid hard stop if one of the post sync connectors errors out
48fbadf
to
7f743d9
Compare
@vossen-adobe I've made the changes you've requested and have resolved the merge conflicts. Please take another look when you get a chance. |
0c8ad26
to
067e5dd
Compare
@vossen-adobe Ready for another look |
Notes:
Fixes #551