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

idrange-add: properly handle empty --dom-name option #667

Closed
wants to merge 1 commit into from

Conversation

flo-renaud
Copy link
Contributor

@flo-renaud flo-renaud commented Mar 28, 2017

When idrange-add is called with --dom-name=, the CLI exits with
ipa: ERROR: an internal error has occurred
This happens because the code checks if the option is provided but does not
check if the value is None.

We need to handle empty dom-name as if the option was not specified.

https://pagure.io/freeipa/issue/6404

@flo-renaud flo-renaud requested a review from stlaz April 4, 2017 14:20
@stlaz stlaz self-assigned this Apr 4, 2017
@stlaz
Copy link
Contributor

stlaz commented Apr 4, 2017

LGTM, except you're talking about idrange-mod in the commit message but are fixing idrange-add (idrange-mod does not have the option at all).

When idrange-add is called with --dom-name=, the CLI exits with
ipa: ERROR: an internal error has occurred
This happens because the code checks if the option is provided but does not
check if the value is None.

We need to handle empty dom-name as if the option was not specified.

https://pagure.io/freeipa/issue/6404
@flo-renaud flo-renaud changed the title idrange-mod: properly handle empty --dom-name option idrange-add: properly handle empty --dom-name option Apr 4, 2017
@flo-renaud
Copy link
Contributor Author

Hi @stlaz
I fixed the commit message.

In contrary to what I told you offline, you need to configure an AD trust with ipa-adtrust-install and ipa trust-add ... in order to reproduce the original issue. My bad...

@stlaz
Copy link
Contributor

stlaz commented Apr 5, 2017

@flo-renaud That's completely OK :)
I thought we could probably add an assert to CIDict.__contains__() method since I realize the issue was somewhere else than this fixed check, but the current situation fails verbosely enough so that's probably fine.
ACK.

@stlaz stlaz added the ack Pull Request approved, can be merged label Apr 5, 2017
@martbab
Copy link
Contributor

martbab commented Apr 5, 2017

@flo-renaud can you please add a test case for this to ipatests/test_xmlrpc/test_range_plugin.py so that we do not regress in the future?

@tkrizek
Copy link
Contributor

tkrizek commented Apr 5, 2017

master:

  • 70743c8 idrange-add: properly handle empty --dom-name option

ipa-4-5:

  • 077a615 idrange-add: properly handle empty --dom-name option

@tkrizek tkrizek added the pushed Pull Request has already been pushed label Apr 5, 2017
@tkrizek tkrizek closed this Apr 5, 2017
@flo-renaud
Copy link
Contributor Author

@martbab
thank you for the suggestion. The new test is available in PR #692

@flo-renaud flo-renaud deleted the t6404 branch April 11, 2017 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants