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 log messages for IP checks during client install #92

Closed
wants to merge 1 commit into from

Conversation

nicki-krizek
Copy link
Contributor

@nicki-krizek nicki-krizek commented Sep 19, 2016

The added log messages allow easier debugging of
IP related issues during ipa-client-install.

https://fedorahosted.org/freeipa/ticket/6331

continue
root_logger.debug('IP check sucessfull: %s' % ip['addr'])
except ValueError as e:
root_logger.debug('IP check failed: %s' % str(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we need to log success here. Btw: successful spells with two 'c' and one 'l' :)

Also, you do not need to do str(e) here as this conversion is done anyway by Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be useful to log success as well. I decided to implement it, because it was suggested in the ticket. Will fix the typo.

@@ -1569,8 +1569,9 @@ def get_local_ipaddresses(iface=None):
for ip in if_addrs.get(family, []):
try:
ips.append(ipautil.CheckedIPAddress(ip['addr']))
except ValueError:
continue
root_logger.debug('IP check sucessfull: %s' % ip['addr'])
Copy link
Member

Choose a reason for hiding this comment

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

A small nitpick - you misspelled sucessfull

The added log messages allow easier debugging of
IP related issues during ipa-client-install.

https://fedorahosted.org/freeipa/ticket/6331
Copy link
Contributor

@stlaz stlaz left a comment

Choose a reason for hiding this comment

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

There are some cases where CheckedIPAddress would not show the failing IP address but these cases should hopefully not appear here => ACK.

@stlaz stlaz added the ack Pull Request approved, can be merged label Sep 20, 2016
@martbab
Copy link
Contributor

martbab commented Sep 21, 2016

@martbab martbab added the pushed Pull Request has already been pushed label Sep 21, 2016
@martbab martbab closed this Sep 21, 2016
@nicki-krizek nicki-krizek deleted the t6331 branch September 22, 2016 12:22
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
Development

Successfully merging this pull request may close these issues.

4 participants