Skip to content

Conversation

@retr0h
Copy link

@retr0h retr0h commented Jan 5, 2015

The CONF.ldap.builtin_users is configured as a comma delimited string, and
needs to be converted to a list before comparison.

Closes-rally-bug: DE778
Not-in-upstream: true

The `CONF.ldap.builtin_users` is configured as a comma delimited string, and
needs to be converted to a list before comparison.

Closes-rally-bug: DE778
Not-in-upstream: true
@scpham
Copy link

scpham commented Jan 5, 2015

👍

1 similar comment
@UshF
Copy link

UshF commented Jan 5, 2015

👍

Choose a reason for hiding this comment

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

Any chance this would be a type other than a string? This would result in a syntax error if say CONF.ldap.builtin_users a number and an attribute error if CONF.ldap.builtin_users was None. I am unsure how the keystone config behaves so it may do sane defaults for example if CONF.ldap.builtin_users was not specified in the config.

@schoksey
Copy link

schoksey commented Jan 5, 2015

This has been addressed as part of the fix in PR - #5

Not sure why it hasn't been considered for merge instead?

@retr0h
Copy link
Author

retr0h commented Jan 5, 2015

@schoksey we are discussing it now.

@signed8bit
Copy link

+1

@retr0h
Copy link
Author

retr0h commented Jan 5, 2015

@schoksey we opted to go this route unless told otherwise. Your pull request looks good, and we can work on merging that. However, it has changes outside the scope of what was reported in DE778. If we wish to get those changes in we will need unit tests, and a story associated with this feature/perf enhancements.

@schoksey
Copy link

schoksey commented Jan 5, 2015

makes sense. will work on a separate user story/unit test creation for PR - #5

alop added a commit that referenced this pull request Jan 12, 2015
Compare against a list not a string
@alop alop merged commit 67f9619 into CiscoSystems:cis-havana Jan 12, 2015
@retr0h retr0h mentioned this pull request Jan 29, 2015
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.

6 participants