Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve
On 01/15/2014 06:24 PM, Alexander Bokovoy wrote: [...] Thanks to Sumit, here is updated patch because I forgot to run makeapi ;( :) You should tell the computer to remind you next time :) For the record, I have this in my .git/hooks/post-commit: ./makeapi git status --short Runs makeapi, and alerts me of any non-committed changes (which include API.txt if that changed). Works quite well in my particular workflow. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve
On Wed, 15 Jan 2014, Alexander Bokovoy wrote:
Hi!
When looking into https://fedorahosted.org/freeipa/ticket/4113, I
decided to use output.standard_list_of_entries instead of a locally
defined list of entries. This solves the problem with wrong exit code in
CLI when non-resolvable SID is given, but only for a single SID. If
multiple SID specified and some of them were not resolved, the exit code
will still be 0 (success) but truncated flag will be set. This
corresponds to the framework behavior in other cases.
Thanks to Sumit, here is updated patch because I forgot to run makeapi
;(
:)
--
/ Alexander Bokovoy
From 972afcc0a23f067249c505824377581c28812733 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy aboko...@redhat.com
Date: Wed, 15 Jan 2014 17:51:25 +0200
Subject: [PATCH 2/2] trust-resolve: improve output by using
standard_list_of_entries
Use output.standard_list_of_entries instead of own output format
gives benefit of returning proper process exit code in CLI.
https://fedorahosted.org/freeipa/ticket/4113
---
API.txt | 5 -
ipalib/plugins/trust.py | 12 +++-
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/API.txt b/API.txt
index a6c3aed..fc5cbc6 100644
--- a/API.txt
+++ b/API.txt
@@ -3666,12 +3666,15 @@ output: Entry('result', type 'dict', Gettext('A
dictionary representing an LDA
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: Output('value', type 'unicode', None)
command: trust_resolve
-args: 0,4,1
+args: 0,4,4
option: Flag('all', autofill=True, cli_name='all', default=False,
exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False,
exclude='webui')
option: Str('sids+', csv=True)
option: Str('version?', exclude='webui')
+output: Output('count', type 'int', None)
output: ListOfEntries('result', (type 'list', type 'tuple'), Gettext('A
list of LDAP entries', domain='ipa', localedir=None))
+output: Output('summary', (type 'unicode', type 'NoneType'), None)
+output: Output('truncated', type 'bool', None)
command: trust_show
args: 1,4,3
arg: Str('cn', attribute=True, cli_name='realm', multivalue=False,
primary_key=True, query=True, required=True)
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 3d412c9..3809f8b 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -992,14 +992,13 @@ class trust_resolve(Command):
Str('sid', label= _('SID')),
)
-has_output = (
-output.ListOfEntries('result'),
-)
+has_output = output.standard_list_of_entries
+msg_summary = _('Resolved %(count)d security identifiers')
def execute(self, *keys, **options):
result = list()
if not _nss_idmap_installed:
-return dict(result=result)
+return dict(result=result, count=0, truncated=True)
try:
sids = map(lambda x: str(x), options['sids'])
xlate = pysss_nss_idmap.getnamebysid(sids)
@@ -1012,7 +1011,10 @@ class trust_resolve(Command):
except ValueError, e:
pass
-return dict(result=result)
+len_sids = len(options['sids'])
+len_result = len(result)
+truncated = len_sids != len_result
+return dict(result=result, count=len_result, truncated=truncated)
api.register(trust_resolve)
--
1.8.4.2
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve
On Wed, Jan 15, 2014 at 07:24:00PM +0200, Alexander Bokovoy wrote: On Wed, 15 Jan 2014, Alexander Bokovoy wrote: Hi! When looking into https://fedorahosted.org/freeipa/ticket/4113, I decided to use output.standard_list_of_entries instead of a locally defined list of entries. This solves the problem with wrong exit code in CLI when non-resolvable SID is given, but only for a single SID. If multiple SID specified and some of them were not resolved, the exit code will still be 0 (success) but truncated flag will be set. This corresponds to the framework behavior in other cases. Thanks to Sumit, here is updated patch because I forgot to run makeapi ;( :) Currently I see: [sbose@ipa18-devel freeipa]$ ipa trust-resolve --sids sdfasdf --- Resolved 0 security identifiers --- Number of entries returned 0 [sbose@ipa18-devel freeipa]$ echo $? 1 Would it be possible to return only one of the summaries to the user? Otherwise the patch works as expected and the output is better than the empty one before. bye, Sumit -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve
On Wed, 15 Jan 2014, Sumit Bose wrote: On Wed, Jan 15, 2014 at 07:24:00PM +0200, Alexander Bokovoy wrote: On Wed, 15 Jan 2014, Alexander Bokovoy wrote: Hi! When looking into https://fedorahosted.org/freeipa/ticket/4113, I decided to use output.standard_list_of_entries instead of a locally defined list of entries. This solves the problem with wrong exit code in CLI when non-resolvable SID is given, but only for a single SID. If multiple SID specified and some of them were not resolved, the exit code will still be 0 (success) but truncated flag will be set. This corresponds to the framework behavior in other cases. Thanks to Sumit, here is updated patch because I forgot to run makeapi ;( :) Currently I see: [sbose@ipa18-devel freeipa]$ ipa trust-resolve --sids sdfasdf --- Resolved 0 security identifiers --- Number of entries returned 0 [sbose@ipa18-devel freeipa]$ echo $? 1 Would it be possible to return only one of the summaries to the user? Otherwise the patch works as expected and the output is better than the empty one before. May be invert summary and tell how many security identifiers were not resolved? -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve
- Original Message - From: Alexander Bokovoy aboko...@redhat.com To: Sumit Bose sb...@redhat.com Cc: freeipa-devel@redhat.com Sent: Wednesday, January 15, 2014 7:00:57 PM Subject: Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve On Wed, 15 Jan 2014, Sumit Bose wrote: On Wed, Jan 15, 2014 at 07:24:00PM +0200, Alexander Bokovoy wrote: On Wed, 15 Jan 2014, Alexander Bokovoy wrote: Hi! When looking into https://fedorahosted.org/freeipa/ticket/4113, I decided to use output.standard_list_of_entries instead of a locally defined list of entries. This solves the problem with wrong exit code in CLI when non-resolvable SID is given, but only for a single SID. If multiple SID specified and some of them were not resolved, the exit code will still be 0 (success) but truncated flag will be set. This corresponds to the framework behavior in other cases. Thanks to Sumit, here is updated patch because I forgot to run makeapi ;( :) Currently I see: [sbose@ipa18-devel freeipa]$ ipa trust-resolve --sids sdfasdf --- Resolved 0 security identifiers --- Number of entries returned 0 [sbose@ipa18-devel freeipa]$ echo $? 1 Would it be possible to return only one of the summaries to the user? Otherwise the patch works as expected and the output is better than the empty one before. May be invert summary and tell how many security identifiers were not resolved? I am personally not convinced this is the right way to fix #4113, for several reasons: 1) The output modification will most probably break FreeIPA 3.2.x or FreeIPA 3.3.x clients who expect different output (the command was introduced in https://fedorahosted.org/freeipa/ticket/3302). 2) I do not think this output is really giving better experience for users. When I get 0 results, does it mean that SID is wrong? Or it is correct but not existent in AD? Or is it correct, existent in AD but SSSD is broken? Instead of checking $?, I would rather expect appropriate errors to be returned - errors.NotFound, errors.ValidationError. Maybe we should return entries for all SIDs but instead of filling sid, name and type for each entry, we would fill sid and error with appropriate error. Would that help? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve
On Wed, 15 Jan 2014, Martin Kosek wrote: - Original Message - From: Alexander Bokovoy aboko...@redhat.com To: Sumit Bose sb...@redhat.com Cc: freeipa-devel@redhat.com Sent: Wednesday, January 15, 2014 7:00:57 PM Subject: Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve On Wed, 15 Jan 2014, Sumit Bose wrote: On Wed, Jan 15, 2014 at 07:24:00PM +0200, Alexander Bokovoy wrote: On Wed, 15 Jan 2014, Alexander Bokovoy wrote: Hi! When looking into https://fedorahosted.org/freeipa/ticket/4113, I decided to use output.standard_list_of_entries instead of a locally defined list of entries. This solves the problem with wrong exit code in CLI when non-resolvable SID is given, but only for a single SID. If multiple SID specified and some of them were not resolved, the exit code will still be 0 (success) but truncated flag will be set. This corresponds to the framework behavior in other cases. Thanks to Sumit, here is updated patch because I forgot to run makeapi ;( :) Currently I see: [sbose@ipa18-devel freeipa]$ ipa trust-resolve --sids sdfasdf --- Resolved 0 security identifiers --- Number of entries returned 0 [sbose@ipa18-devel freeipa]$ echo $? 1 Would it be possible to return only one of the summaries to the user? Otherwise the patch works as expected and the output is better than the empty one before. May be invert summary and tell how many security identifiers were not resolved? I am personally not convinced this is the right way to fix #4113, for several reasons: 1) The output modification will most probably break FreeIPA 3.2.x or FreeIPA 3.3.x clients who expect different output (the command was introduced in https://fedorahosted.org/freeipa/ticket/3302). This command is only used within Web UI. It is not supposed to be used by CLI for anything, it was marked for CLI only for some QE request at the time we still had issues with SID resolution in sssd. I'd rather mark it NO_CLI=True because it is really a tool for Web UI asynchronous resolution of SIDs to names for external members of groups. 2) I do not think this output is really giving better experience for users. When I get 0 results, does it mean that SID is wrong? Or it is correct but not existent in AD? Or is it correct, existent in AD but SSSD is broken? Instead of checking $?, I would rather expect appropriate errors to be returned - errors.NotFound, errors.ValidationError. Maybe we should return entries for all SIDs but instead of filling sid, name and type for each entry, we would fill sid and error with appropriate error. Would that help? This will be overkill for Web UI where it is really not needed. All we need is a list of resolved SIDs as names. Missing name will simply leave SID in the UI. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0133 Use standard_list_of_entries for trust-resolve
On Wed, 15 Jan 2014, Alexander Bokovoy wrote:
I am personally not convinced this is the right way to fix #4113, for
several reasons:
1) The output modification will most probably break FreeIPA 3.2.x or
FreeIPA 3.3.x clients who expect different output (the command was
introduced in https://fedorahosted.org/freeipa/ticket/3302).
This command is only used within Web UI. It is not supposed to be used
by CLI for anything, it was marked for CLI only for some QE request at
the time we still had issues with SID resolution in sssd. I'd rather
mark it NO_CLI=True because it is really a tool for Web UI asynchronous
resolution of SIDs to names for external members of groups.
Also note that from Web UI side the output change proposed in this patch
is an extension of what Web UI expects. All the old fields are the same,
it is only a bit of metadata added alongside with previously provided
'result' array. Since Web UI references data by field name, no change is
needed:
sidxlate_command.on_success = function(data, text_status, xhr) {
for (var i=0; i data.result.result.length; i++) {
var entry = data.result.result[i];
if (entry.sid[0] in xlate) {
xlate[entry.sid[0]].resolve(entry.name[0]);
}
}
};
--
/ Alexander Bokovoy
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
