[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

jhrozek commented:
"""
* b78febe4c579f86f8007a27599605d1eb9f97a62
* 213048fd9a5e800deb74cb5b7f0eaf465945c640
* f9bac02756aa05cc9c6ac07ae581dba67240c1a4
* dae798231fc2c575f213785768bc24ed765ba243
* ed518f61f1a5d4cf5d87eec492c158725a73d6a1
* a3faad0e4dc1ca4473746c3822ecfc5aed876e6d

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-297515948
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
Added the "Accepted" label according to @pbrezina's ACK,
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-297432010
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

pbrezina commented:
"""
Ack. Thank you for your patience.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-297304243
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
On Tue, Apr 25, 2017 at 1:56 PM, Pavel Březina 
wrote:

> Yes, I think we should log it so we know what's used when we will debug
> issues :-)
>

Changes done and patchset updated!


> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> , or mute
> the thread
> 
> .
>

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-297014695
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

pbrezina commented:
"""
Yes, I think we should log it so we know what's used when we will debug issues 
:-)
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-297006716
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
On Tue, Apr 25, 2017 at 12:37 PM, Pavel Březina 
wrote:

> Thank you. Last trivial change, you have typo in commit message (domaiN):
> RESPONDER_COMMON: Improve domaiN_resolution_order debug messages
>
> Thank you for the debug messages, it's good to know where it comes from.
> The local resolution order from the configuration file is visible in the
> logs. I don't have currently and IPA setup supporting this prepared -- do
> we print the resolution order from IPA as well?
>

We print ""Using domain_resolution_order from IPA ID View\n"" or "Using
domain_resolution_order from IPA Config\n" and that's it.

I don't specifically print the order being used (and I'm not exactly sure
we should). Do you think we should?


> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> , or mute
> the thread
> 
> .
>

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-296994058
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

pbrezina commented:
"""
Thank you. Last trivial change, you have typo in commit message (domaiN):
`RESPONDER_COMMON: Improve domaiN_resolution_order debug messages`

Thank you for the debug messages, it's good to know where it comes from. The 
local resolution order from the configuration file is visible in the logs. I 
don't have currently and IPA setup supporting this prepared -- do we print the 
resolution order from IPA as well?
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-296990547
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
Patch set updated according to @pbrezina's comments.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-296793840
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

pbrezina commented:
"""
Functional ack, but please, do the same change also for 
`sss_resp_new_cr_domains_from_ipa_id_view`.

I would also welcome some debug messages so we can known that shortname lookup 
was used etc. and some comments in `cache_req_domain_new_list_from_string_list` 
describing what the for cycles do.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-296618933
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
@pbrezina: Yeah, not so easy to make the bad habits to go away :-)

Here's a diff of the changes made after your review:
```
[ffidenci@pessoa x86_64]$ git diff
diff --git a/src/responder/common/cache_req/cache_req_domain.c 
b/src/responder/common/cache_req/cache_req_domain.c
index 21a4224..86a88ef 100644
--- a/src/responder/common/cache_req/cache_req_domain.c
+++ b/src/responder/common/cache_req/cache_req_domain.c
@@ -128,11 +128,10 @@ cache_req_domain_new_list_from_domain_resolution_order(
 struct cache_req_domain **_cr_domains)
 {
 TALLOC_CTX *tmp_ctx;
+struct cache_req_domain *cr_domains;
 char **list = NULL;
 errno_t ret;
 
-*_cr_domains = NULL;
-
 tmp_ctx = talloc_new(NULL);
 if (tmp_ctx == NULL) {
 return ENOMEM;
@@ -151,9 +150,9 @@ cache_req_domain_new_list_from_domain_resolution_order(
 }
 }
 
-*_cr_domains = cache_req_domain_new_list_from_string_list(mem_ctx, domains,
-  list);
-if (*_cr_domains == NULL) {
+cr_domains = cache_req_domain_new_list_from_string_list(mem_ctx, domains,
+list);
+if (cr_domains == NULL) {
 ret = ENOMEM;
 DEBUG(SSSDBG_OP_FAILURE,
   "cache_req_domain_new_list_from_domain_resolution_order() "
@@ -162,6 +161,7 @@ cache_req_domain_new_list_from_domain_resolution_order(
 goto done;
 }
 
+*_cr_domains = cr_domains;
 ret = EOK;
 
 done:
```
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-296159240
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

pbrezina commented:
"""
Looking at the first patch, it is a bad habit to assign to an output variable 
on failures. Whenever possible, we are trying to avoid it by duplicating the 
variable for local use and assigning to it only in success branch. I.e. not:

```c
errno_t
cache_req_domain_new_list_from_domain_resolution_order([...] struct 
cache_req_domain **_cr_domains)
{
errno_t ret;

*_cr_domains = NULL;

[...]

*_cr_domains = cache_req_domain_new_list_from_string_list(mem_ctx, domains, 
list);
if (*_cr_domains == NULL) {
ret = ENOMEM;
goto done;
}

ret = EOK;

done:
talloc_free(tmp_ctx);
return ret;
}
```
But:
```c
errno_t
cache_req_domain_new_list_from_domain_resolution_order([...] struct 
cache_req_domain **_cr_domains)
{
struct cache_req_domain *cr_domains;
errno_t ret;

cr_domains = cache_req_domain_new_list_from_string_list(mem_ctx, domains, 
list);
if (cr_domains == NULL) {
ret = ENOMEM;
goto done;
}

*_cr_domains = cr_domains;
ret = EOK;

done:
talloc_free(tmp_ctx);
return ret;
}
```

Besides this little nitpick, the patches look good to me.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-296156663
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
CI results for the new patchset: 
http://sssd-ci.duckdns.org/logs/job/67/95/summary.html
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-295175418
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
Okay, new patchset updated addressing @lslebodn's comments.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-294835307
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
On Tue, Apr 18, 2017 at 2:12 PM, lslebodn  wrote:

> On (18/04/17 03:47), fidencio wrote:
> >On Tue, Apr 18, 2017 at 11:33 AM, lslebodn 
> wrote:
> >
> >> Removing unit test is not acceptable without writing new one.
> >>
> >
> >Lukáš,
> >
> >Maybe I wasn't clear enough in the "NSS/TESTS: Improve non-fqnames tests"
> >commit message.
> >
> >I do NOT remove any test per si.
> >The situation before was:
> >- I was using the very same setup() function and setting
> subdomain->fqnames
> >
> Then it would be good to have common_setup function
> or call one setup function from another + additional modifications.
>
> >The current situation is:
> >- Now I set subdomain->fqnames = false in the newly introduced setup()
> >function, which allows me to use the tests from where I copied, pasted and
> >changed the code to set subdomain->fqnames = false.
> >
>
> It is not just about `subdomain->fqnames = false`
> There are more changes between `test_nss_getgrnam_members_subdom`
> and `test_nss_getgrnam_members_subdom`.
>
> ```
> -void test_nss_getgrnam_members_subdom(void **state)
> +void test_nss_getgrnam_members_subdom_nonfqnames(void **state)
> {
> errno_t ret;
>
> - mock_input_user_or_group("testsubdomgroup@"TEST_SUBDOM_NAME);
> + nss_test_ctx->subdom->fqnames = false;
> +
> + mock_input_user_or_group("testsubdomgroup");
> + mock_account_recv_simple();
> will_return(__wrap_sss_packet_get_cmd, SSS_NSS_GETGRNAM);
> will_return_always(__wrap_sss_packet_get_body, WRAP_CALL_REAL);
> ```
>
> And you removed valid test case without equivalent replacement.
>


Now I've seen what I did wrong. :-)

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-294828423
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

lslebodn commented:
"""
On (18/04/17 03:47), fidencio wrote:
>On Tue, Apr 18, 2017 at 11:33 AM, lslebodn  wrote:
>
>> Removing unit test is not acceptable without writing new one.
>>
>
>Lukáš,
>
>Maybe I wasn't clear enough in the "NSS/TESTS: Improve non-fqnames tests"
>commit message.
>
>I do NOT remove any test per si.
>The situation before was:
>- I was using the very same setup() function and setting subdomain->fqnames
>
Then it would be good to have common_setup function
or call one setup function from another + additional modifications.

>The current situation is:
>- Now I set subdomain->fqnames = false in the newly introduced setup()
>function, which allows me to use the tests from where I copied, pasted and
>changed the code to set subdomain->fqnames = false.
>

It is not just about `subdomain->fqnames = false`
There are more changes between `test_nss_getgrnam_members_subdom`
and `test_nss_getgrnam_members_subdom`.

```
-void test_nss_getgrnam_members_subdom(void **state)
+void test_nss_getgrnam_members_subdom_nonfqnames(void **state)
 {
 errno_t ret;
 
-mock_input_user_or_group("testsubdomgroup@"TEST_SUBDOM_NAME);
+nss_test_ctx->subdom->fqnames = false;
+
+mock_input_user_or_group("testsubdomgroup");
+mock_account_recv_simple();
 will_return(__wrap_sss_packet_get_cmd, SSS_NSS_GETGRNAM);
 will_return_always(__wrap_sss_packet_get_body, WRAP_CALL_REAL);
```

And you removed valid test case without equivalent replacement.

LS

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-294812283
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
On Tue, Apr 18, 2017 at 11:33 AM, lslebodn  wrote:

> Removing unit test is not acceptable without writing new one.
>

Lukáš,

Maybe I wasn't clear enough in the "NSS/TESTS: Improve non-fqnames tests"
commit message.

I do NOT remove any test per si.
The situation before was:
- I was using the very same setup() function and setting subdomain->fqnames
= false in the tests that I've copied and pasted.

The current situation is:
- Now I set subdomain->fqnames = false in the newly introduced setup()
function, which allows me to use the tests from where I copied, pasted and
changed the code to set subdomain->fqnames = false.

In the end, some code ended up removing which allowed me to use the
previous tests and remove the tests that were introduced with the first
part of these patches (but with a new setup() function).

So, looking carefully at the patch and the commit message you'll realize
that no tests were removed after all.

Does that make sense to you?

I'm removing the "Changes requested" label.

> —
>
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> , or mute
> the thread
> 
> .
>

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-294778275
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

lslebodn commented:
"""
Removing unit test is not acceptable without writing new one.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-294746901
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

lslebodn commented:
"""
On (13/04/17 02:17), Jakub Hrozek wrote:
>retest this please
>
It is just wasting of time unless somebody will fix
enumeration issues.
https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org/thread/BWNWWJE4WSOHG4ZBVCPHWS6JWAJOOQZZ/#EIWV67IXH25HYW4XVF2Q3I2NRB3AHC43

LS

"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-294745462
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
CI passed: http://sssd-ci.duckdns.org/logs/job/67/93/summary.html
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-294445018
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
retest this please
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-294443065
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

jhrozek commented:
"""
retest this please
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-293837126
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#235][comment] Allow using the "shortnames" feature without requiring any configuration from the client side

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/235
Title: #235: Allow using the "shortnames" feature without requiring any 
configuration from the client side

fidencio commented:
"""
retest please
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/235#issuecomment-293834628
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org