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 check for removing last KRA server #553

Closed
wants to merge 2 commits into from
Closed

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Mar 8, 2017

This patchset adds a check for removal of a last KRA server + adds a message about there only being one KRA to WebUI.

@stlaz
Copy link
Contributor Author

stlaz commented Mar 8, 2017

Hm, I forgot that KRA is the only IPA component that has a standalone uninstaller, this is therefore only a partial fix.

@MartinBasti
Copy link
Contributor

@stlaz I wrote it to ticket

@stlaz
Copy link
Contributor Author

stlaz commented Mar 8, 2017

@MartinBasti ah, sorry, I completely overlooked it. The current PR version implements your suggestion.

@MartinBasti
Copy link
Contributor

Please create a separate commit for KRA Uninstall

@stlaz
Copy link
Contributor Author

stlaz commented Mar 8, 2017

Split done.

@MartinBasti
Copy link
Contributor

JFTR: KRA uninstall commit is here #556

Copy link
Contributor

@MartinBasti MartinBasti left a comment

Choose a reason for hiding this comment

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

I got following error in webUI even I had only one server with both CA and KRA installed, this shouldn't happened

Warning: Only One CA/KRA Server Detected

It is strongly recommended to keep the KRA services installed on more than one server.

It is known bug that the warning is shown.
But in this case, there should be both warnings for CA and KRA, so it is broken by your patch, I got only KRA warning.

var dialog = IPA.dialog({
name: 'ca_warning',
title: '@i18n:objects.servers.ca_warning_title',
name: 'dogtag_warning',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like to have just one common title

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you dynamically create that list? something like Warning: Only One {roles} Server Detected

@MartinBasti
Copy link
Contributor

Probably you we should fix this before we double number of alerts
https://pagure.io/freeipa/issue/6598

Copy link

@pvomacka pvomacka left a comment

Choose a reason for hiding this comment

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

Please see inline comments.

if (ca_counter != 1 && kra_counter != 1) return;

var messages = [];
if (ca_counter == 1)
Copy link

Choose a reason for hiding this comment

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

Please use '===' instead od '==', two equals might cause weird behavior.

And for better reading of code please use code block { } around commands after if statement. It will work if you have only one command after the condition, but in case that the command is not onliner it is better to have it in curly braces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

== can't cause this, we know already that the types are equal as ca_counter is a local variable that is not assigned from any external source.

$type: 'html',
html: text.get('@i18n:objects.servers.ca_warning_message')
});
if (kra_counter == 1)
Copy link

Choose a reason for hiding this comment

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

The same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above.

@pvoborni
Copy link
Member

pvoborni commented Mar 9, 2017

Fix for 6598 in #566

@stlaz
Copy link
Contributor Author

stlaz commented Mar 10, 2017

Reworked how the beg for a service replication worked.

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

master:

  • 670f8fb Add check to prevent removal of last KRA
  • 1e8db4b Add message about last KRA to WebUI Topology view

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Mar 13, 2017
@stlaz stlaz deleted the last_kra branch September 11, 2017 10:47
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