7:24 a.m.
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: opened
PR body:
"""
Some users change the IPA hostname post-install which results in strange bugs. Code change
make sure that the ipa_hostname contains at least one domain component.
Resolves: https://pagure.io/SSSD/sssd/issue/1946
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
7:25 a.m.
New subject: [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
7:25 a.m.
New subject: [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
7:29 a.m.
New subject: [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
10:57 a.m.
New subject: [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
10:57 a.m.
New subject: [sssd PR#326][+Changes requested] 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
Label: +Changes requested
5:51 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
6:09 a.m.
New subject: [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
6:13 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
3:59 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
4:10 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
6:26 a.m.
New subject: [sssd PR#326][-Changes requested] 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
Label: -Changes requested
6:52 a.m.
New subject: [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
3:21 p.m.
New subject: [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
5:23 p.m.
New subject: [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
5:23 p.m.
New subject: [sssd PR#326][+Changes requested] 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
Label: +Changes requested
3:03 a.m.
New subject: [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
7:29 a.m.
New subject: [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
7:33 a.m.
New subject: [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
8:20 a.m.
New subject: [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
8:22 a.m.
New subject: [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
9:04 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
5:20 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
5:22 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
7:30 a.m.
New subject: [sssd PR#326][-Changes requested] 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
Label: -Changes requested
7:11 a.m.
New subject: [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:
"""
retest this please
"""
See the full comment at https://github.com/SSSD/sssd/pull/326#issuecomment-322173917
3:11 p.m.
New subject: [sssd PR#326][+Changes requested] 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
Label: +Changes requested
7:53 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
1:55 p.m.
New subject: [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
5:34 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
5:35 a.m.
New subject: [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
8 a.m.
New subject: [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
6:54 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
7:01 a.m.
New subject: [sssd PR#326][synchronized] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
7:01 a.m.
New subject: [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
2:23 a.m.
New subject: [sssd PR#326][-Changes requested] 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
Label: -Changes requested
2:05 p.m.
New subject: [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
2:05 p.m.
New subject: [sssd PR#326][closed] IPA: check if IPA hostname is a FQDN
URL: https://github.com/SSSD/sssd/pull/326
Author: amitkumar50
Title: #326: IPA: check if IPA hostname is a FQDN
Action: closed
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/326/head:pr326
git checkout pr326
2:05 p.m.
New subject: [sssd PR#326][+Pushed] 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
Label: +Pushed
2415
days inactive
2471
days old
sssd-devel@lists.fedorahosted.org
38 comments
4 participants
participants (4)
-
amitkumar50 -
centos-ci
-
fidencio
-
jhrozek