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

Jump to bottom

Don't allow standalone KRA uninstalls #556

Closed
wants to merge 1 commit into from
Closed

Don't allow standalone KRA uninstalls #556

wants to merge 1 commit into from

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Mar 8, 2017 •
edited by MartinBasti

KRA uninstallation is very likely to break the user's setup. Don't
allow it at least till we can be safely sure we are able to remove
it in a standalone manner without breaking anything.

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

@MartinBasti MartinBasti mentioned this pull request Mar 8, 2017
Closed
@tkrizek tkrizek self-assigned this Mar 9, 2017
tkrizek
tkrizek suggested changes Mar 9, 2017
View reviewed changes
ipaserver/install/ipa_kra_install.py Outdated
return KRAUninstaller
raise RuntimeError(
'Standalone KRA uninstalling was removed in FreeIPA 4.5 as it '
'had never worker properly and only caused issues.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please improve the error message text:
uninstalling -> uninstallation
had -> has
worker -> worked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps uninstallation is the better word here, thanks for finding the typo as well. However, had is used properly, check your grammar ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar has been checked. You're indeed correct :)

ipaserver/install/ipa_kra_install.py Outdated
@@ -83,33 +83,13 @@ def validate_options(self, needs_root=True):
@classmethod
def get_command_class(cls, options, args):
if options.uninstall:
return KRAUninstaller
raise RuntimeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

ipa-kra-install --uninstall prints a traceback instead of just showing the error message.

@tkrizek
Copy link
Contributor

tkrizek commented Mar 9, 2017

You should also remove:

ipaplatform/base/paths.py:313:    IPASERVER_KRA_UNINSTALL_LOG
ipatests/test_integration/tasks.py:71:    host.collect_log(paths.IPASERVER_KRA_UNINSTALL_LOG)
ipatests/test_integration/tasks.py:73:    host.collect_log(paths.IPASERVER_KRA_UNINSTALL_LOG)
ipatests/test_integration/test_vault.py:145:    def test_create_and_retrieve_vault_after_kra_uninstall_on_replica

@stlaz
Copy link
Contributor Author

stlaz commented Mar 9, 2017

Should be fixed now, had to add sys.exit() call not to show traceback 😞

@MartinBasti
Copy link
Contributor

ScriptError didn't work?

@stlaz
Copy link
Contributor Author

stlaz commented Mar 9, 2017

@MartinBasti unfortunately not.

@tkrizek tkrizek added the ack Pull Request approved, can be merged label Mar 9, 2017
@MartinBasti MartinBasti removed the ack Pull Request approved, can be merged label Mar 9, 2017
@MartinBasti
Copy link
Contributor

Waiting for more opinions about removing KRA --uninstall

@pvoborni
Copy link
Member

pvoborni commented Mar 9, 2017 •
edited

OK, so this pr removes --uninstall from ipa-kra-install. Did it work in the past? Or it always broke the installation? AFAIK this workflow was not really tested. If answers are "No, Yes, Yes" then I'm OK with the PR.

MartinBasti
MartinBasti reviewed Mar 9, 2017
View reviewed changes
ipaserver/install/ipa_kra_install.py
return KRAUninstaller
sys.exit(
'ERROR: Standalone KRA uninstallation was removed in '
'FreeIPA 4.5 as it had never worked properly and only caused '
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the wording to ERROR: Standalone KRA uninstallation was removed in FreeIPA 4.5 without extra information about how broken it was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some kind of reason should be provided. Perhaps we can reword it to "...as it was a potential cause of issues."

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Mar 13, 2017
@MartinBasti
Copy link
Contributor

needs rebase

@stlaz
Don't allow standalone KRA uninstalls
568ed7b 
KRA uninstallation is very likely to break the user's setup. Don't
allow it at least till we can be safely sure we are able to remove
it in a standalone manner without breaking anything.

https://pagure.io/freeipa/issue/6538
@stlaz
Copy link
Contributor Author

stlaz commented Mar 13, 2017

Rebased.

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Mar 13, 2017
@MartinBasti
Copy link
Contributor

master:

  • 5d3a0e6 Don't allow standalone KRA uninstalls

@stlaz stlaz deleted the no_kra_uninstall branch September 11, 2017 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinBasti MartinBasti MartinBasti left review comments

@tkrizek tkrizek tkrizek approved these changes

Assignees

@tkrizek tkrizek

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
4 participants
@stlaz @tkrizek @MartinBasti @pvoborni