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

Provide user hint about IP address in IPA install #207

Closed
wants to merge 1 commit into from
Closed

Provide user hint about IP address in IPA install #207

wants to merge 1 commit into from

Conversation

Akasurde
Copy link
Member

@Akasurde Akasurde commented Nov 2, 2016 •
edited by MartinBasti

With this fix, user will be notified about pressing enter
to proceed with IPA installation procedure, if user has
provided valid IP address previously.

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

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

MartinBasti
MartinBasti suggested changes Nov 3, 2016
View reviewed changes
ipaserver/install/installutils.py
while True:
ip = ipautil.user_input("Please provide the IP address to be used for this host name", allow_empty = True)
if ips:
msg += " or leave blank to continue with the installation"
Copy link
Contributor

Choose a reason for hiding this comment

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

You appending text to message in cycle, Please provide the IP address to be used for this host name or leave blank to continue with the installation or leave blank to continue with the installation or leave blank to continue with the installation

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, my bad. I will fix it.

@MartinBasti
Copy link
Contributor

NACK, please see inline comment

I'd use 2 different messages:
1st IP: "Please provide the IP address to be used for this host name:"
2nd and more IP: "Additional IP address (leave blank to continue with installation):"

to be clear that we are appending IP addresses

@MartinBasti MartinBasti self-assigned this Nov 3, 2016
@rsd
Copy link

rsd commented Nov 3, 2016

This happens the case where the user keeps entering a IP address and the UI asks again as if it were ignored.

@MartinBasti
Copy link
Contributor

May I propose following?

+    msg_first = "Please provide the IP address to be used for this host name"
+    msg_other = (
+        "Additional IP address (leave blank to continue with the installation)")
     while True:
-        ip = ipautil.user_input("Please provide the IP address to be used for this host name", allow_empty = True)
+        msg = msg_other if ips else msg_first
+        ip = ipautil.user_input(msg, allow_empty=True)

Please use rather () than \ in future to split string

@Akasurde Akasurde force-pushed the tkt-5949 branch 2 times, most recently from 80fa97a to 8ef85fe Compare November 4, 2016 06:36
MartinBasti
MartinBasti suggested changes Nov 7, 2016
View reviewed changes
ipaserver/install/installutils.py
@@ -268,8 +268,12 @@ def add_record_to_hosts(ip, host_name, conf_file=paths.HOSTS):
def read_ip_addresses():
ips = []
print("Enter the IP address to use, or press Enter to finish.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this print should be removed with this commit

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasti-rh OK.

@MartinBasti
Copy link
Contributor

I realized that this is somehow inconsistent with getting IP addresses for forwarders

DNS forwarders: Enter an IP address for a DNS forwarder, or press Enter to skip

So IMO we should be more consistent and use: Enter an additional IP address, or press Enter to skip

What do you think?

@Akasurde
Copy link
Member Author

Akasurde commented Nov 7, 2016

@mbasti-rh I will change message for DNS forwarders from Enter an IP address for a DNS forwarder,... to Enter an additional IP address, or press Enter to skip.

@MartinBasti
Copy link
Contributor

@Akasurde I would rather keep explicitly DNS forwarder there

@MartinBasti
Copy link
Contributor

What I meant was to change message for host IP address

@Akasurde
Provide user hint about IP address in IPA install
2fc9985 
With this fix, user will be notified about pressing enter
to proceed with IPA installation procedure, if user has
provided valid IP address previously.

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

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@MartinBasti
Copy link
Contributor

LGTM, I'll test later

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Nov 11, 2016
@MartinBasti
Copy link
Contributor

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/28bc54f91dfbd76887180fa67ceecb46977a4fb8

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Nov 11, 2016
@Akasurde Akasurde deleted the tkt-5949 branch November 12, 2016 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinBasti MartinBasti MartinBasti approved these changes

Assignees

@MartinBasti MartinBasti

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
3 participants
@Akasurde @MartinBasti @rsd