Skip to content

V2 for release 2.4 #443

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

Merged
merged 5 commits into from
Feb 6, 2019
Merged

V2 for release 2.4 #443

merged 5 commits into from
Feb 6, 2019

Conversation

shasibhusanJena
Copy link
Contributor

  • removed line from app.py default AssertionException block - added a… …
    … separate code block on config.py file to validate this condition.
    -- doing a comparison with connectors_config. the value so that, don't have to loop through to make a conditional check "Okta" type

Added generic log stmt to retrieve values using LDAP
Added generic log stmt to retrieve values using LDAP
… separate code block on config.py file to validate this condition.
removed connectors_config.accessed_keys loop and checking with container value comparision with JSON object

#416
@adorton-adobe
Copy link
Collaborator

@shasibhusanJena You don't need to make new PR when you make new commits. Just commit to the branch the PR is coming from and they'll be picked up here.

@shasibhusanJena
Copy link
Contributor Author

oh, I was not knowing about it before, that will be easy then. thanks

@adorton-adobe adorton-adobe self-requested a review January 28, 2019 18:08
@adorton-adobe adorton-adobe added this to the v2.4.1 milestone Jan 28, 2019
Copy link
Collaborator

@adorton-adobe adorton-adobe left a comment

Choose a reason for hiding this comment

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

I think you've found the right place to do this check. After doing some testing, I've found that we need to make this check more generic so we always require a directory connector config for all connector types except for CSV. I've also made some other notes - please review.

@@ -323,6 +323,11 @@ def get_directory_connector_options(self, connector_name):
"""
options = {}
connectors_config = self.get_directory_connector_configs()

if ( 'okta' in connectors_config.value) and (connector_name == 'ldap'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I've had more time to look at this, I think we need to make this check more generic. If you do the opposite of what we're checking here - enable the link to the Okta config, but make sure the --connector is "ldap" - then we still get an unhelpful message.

The check should ensure that the correct config file is specified for the connector type. The only exception is CSV, which does not require a configuration file.

Here is how I would modify your check:

if connector_name != 'csv' and connector_name not in connectors_config.value:
  # ... rest of check

You will also need to change the error message - I'll address that separately.


if ( 'okta' in connectors_config.value) and (connector_name == 'ldap'):
if connectors_config.accessed_keys is not None:
raise AssertionException("Failed to match request : for Okta Connector type receiving LDAP as input type from commnand line")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you make the check more generic you'll need to make this message generic as well. I'd do something like this:

raise AssertionException("Config file must be specified for connector type '{}'".format(connector_name))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified. please review

removed connectors_config.accessed_keys and made it more generic to find correct type
Copy link
Collaborator

@adorton-adobe adorton-adobe left a comment

Choose a reason for hiding this comment

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

👍

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.

2 participants