[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN jhrozek commented: """ I fixed the indentation locally and pushed the patches to master: efa0a019f1ede87bcdd4668e70c768b222c30167 fdefac9c4a5c9f2dcc8748ccb736e9a6910c2365 """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-329266115 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN amitkumar50 commented: """ @jhrozek Thanks. Updated. """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-327461874 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN jhrozek commented: """ @amitkumar50 sorry for noticing the check is misplaced earlier. Please move it and I'll ack.. """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-327167852 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN amitkumar50 commented: """ @jhrozek Agree with you. Done. Thanks """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-327137863 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN jhrozek commented: """ I have one more request - please don't make the check fatal. Just report a failure and move on. There might be deployments which currently work, but we would break them with this patch. So please remove the ` return ERR_INVALID_CONFIG;` line and I'll ack. """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-327015101 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN fidencio commented: """ @amitkumar50, that's good for me as well. Please, update the PR with the changes :-) """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-321253610 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN amitkumar50 commented: """ @fidencio I am not clear why you used term "newly specified". Though without it "The hostname must be fully qualified." would be good too. """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-321252952 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN fidencio commented: """ @amitkumar50: what do you think about "The newly specified hostname must be fully qualified."? """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-321241893 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN amitkumar50 commented: """ @fidencio sssd-ipa man page ipa_hostname entry: > ipa_hostname (string) > Optional. May be set on machines where the hostname(5) does not > reflect the fully qualified name used in the IPA domain to identify this host. How good it would be to append following text to above description: > If specified, hostname should be fully qualified name. """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-321240900 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN amitkumar50 commented: """ @fidencio Thanks for suggestion. Yes indeed That would be Good.. I will do the change.. """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-321183687 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN fidencio commented: """ @amitkumar50 ... this patch itself looks good and works as expected. I'm wondering, though, whether the sssd-ipa man page should be updated as well (as a first patch) in order to mention that the ipa_hostname option **must** be fully-qualified and then, in this patch, mention something like "ipa_hostname must be a fully-qualified name, please refer to the sssd-ipa manpage for more information" to this patch you've submitted. Let me know if you're willing to make the change. In a negative case I can provide the patch and rebase yours on top of mine. Thanks for the contribution! (I'm adding the "Changes Requested" label as per this review). """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-320797156 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN fidencio commented: """ retest this please """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-320769797 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN amitkumar50 commented: """ @fidencio > All checks have failed Is there anything to be done from my end.. Or this Build check needed to be run again? """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-320229990 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN fidencio commented: """ @amitkumar50, whenever you're editing your pull-request with the suggestions given by the reviewers, please, squash the patches that contain the fixes into the previous patch (the one that had to be changed). Please, re-submit the PR. """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-319037975 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Title: #326: IPA: check if IPA hostname is a FQDN
fidencio commented:
"""
@amitkumar50: I've tested the patch, it works, but I'll request some changes in
it.
Let me write you here a few general recommendations based on a few different
parts of the code:
- Commit message: Although the commit message itself is good, please, don't use
more than 72 characters in the body (please, see
https://github.com/SSSD/sssd/blob/master/.git-commit-template)
- Coding style:
- Instead of doing `if(!ipa_check_fqdn(ipa_hostname)){ `, please, do `if
(!ipa_check_fqdn(ipa_hostname)) {`;
- Be careful about the alignment. So, instead of doing:
```
DEBUG(SSSDBG_CRIT_FAILURE,
"ipa_hostname is not Fully Qualified Domain Name.\n");
```
please, do:
```
DEBUG(SSSDBG_CRIT_FAILURE,
"ipa_hostname is not Fully Qualified Domain Name.\n");
```
- Only use implicit checks against bool. For instance, instead of `if(ret){`,
please, do `if (ret != NULL) {`
And last but not least, I do believe this function could be an internal one
inside src/providers/ipa/ipa_init.c as I do believe the check could be done
only in the ipa_init_server_mode() function.
Thanks a lot for the patch and I'm setting the "Changes Requested" label as per
this review.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/326#issuecomment-318405884
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN fidencio commented: """ ok to test """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-316370187 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN centos-ci commented: """ Can one of the admins verify this patch? """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-316369178 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#326][comment] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326 Title: #326: IPA: check if IPA hostname is a FQDN centos-ci commented: """ Can one of the admins verify this patch? """ See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-316369182 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
