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

Add fix for no-hbac-allow option in server install #116

Closed
wants to merge 1 commit into from

Conversation

Akasurde
Copy link
Member

@Akasurde Akasurde commented Sep 26, 2016

Fixes: https://fedorahosted.org/freeipa/ticket/6357

Signed-off-by: Abhijeet Kasurde akasurde@redhat.com

@tkrizek tkrizek self-assigned this Sep 26, 2016
@tkrizek
Copy link
Contributor

tkrizek commented Sep 26, 2016

--no_hbac_allow option should remain as an undocumented option for backwards compatibility.

@Akasurde
Copy link
Member Author

@tomaskrizek Should I remove 'no_hbac_allow' option from ipa-server-install man page then ?

@tkrizek
Copy link
Contributor

tkrizek commented Sep 26, 2016

@Akasurde The man page is correct. However, we can no longer use the command

ipa-server-install --no_hbac_allow

if you simply rename the option. We need to keep this option for backwards compatibility and add a new option --no-hbac-allow. Internally, no_hbac_allow should only be an alias to no-hbac-allow. As a result, both of the following commands should work:

ipa-server-install --no-hbac-allow
ipa-server-install --no_hbac_allow

With --no-hbac-allow being the preferred and documented option, while --no_hbac_allow simply remains for backwards compatibility.

I think the correct way to fix the issue is to add a new option and then make sure both the options have the same effect when used.

@@ -1294,7 +1294,7 @@ def idmax(self):
no_hbac_allow = Knob(
bool, False,
description="Don't install allow_all HBAC rule",
cli_name='no_hbac_allow',
cli_name='no-hbac-allow',
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also add cli_aliases=['no_hbac_allow'] for backward compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcholast Do I need to run makeapi too ?

Copy link
Contributor

@stlaz stlaz Oct 11, 2016

Choose a reason for hiding this comment

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

@Akasurde The easiest way to find out is to run it and do git diff. If there are changes in API.txt, you should have run it ;)
Just please do so that we can close this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stlaz OK. I will do it soon.

This PR brings uniformity in option provided by no-hbac-allow
and other options present in IPA server install script

Fixes https://fedorahosted.org/freeipa/ticket/6357

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@Akasurde Akasurde changed the title Added fix for no-hbac-allow option in server install script Add fix for no-hbac-allow option in server install Oct 14, 2016
@tkrizek
Copy link
Contributor

tkrizek commented Oct 14, 2016

LGTM, thanks for the patch!

@tkrizek tkrizek added the ack Pull Request approved, can be merged label Oct 14, 2016
@Akasurde
Copy link
Member Author

@tomaskrizek @stlaz @jcholast Thanks for review comments.

@MartinBasti
Copy link
Contributor

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Oct 18, 2016
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
5 participants