[apparmor] [PATCH 2/2] Add JSON interface to UI_Changes

[Goldwyn Rodrigues]

On 10/25/2017 05:20 PM, Christian Boltz wrote:
> Hello,
> 
> Am Montag, 23. Oktober 2017, 12:38:34 CEST schrieb Goldwyn Rodrigues:
>> From: Goldwyn Rodrigues <rgoldwyn at suse.com>
>>
>> Provides the filename in the json format, which can be
>> directly read by Yast. Increased the protocol version; perhaps
>> it should go in the next release.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.com>
>> ---
>>  utils/apparmor/ui.py | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/utils/apparmor/ui.py b/utils/apparmor/ui.py
>> index be07b28a..2afbd5b1 100644
>> --- a/utils/apparmor/ui.py
>> +++ b/utils/apparmor/ui.py
>> @@ -45,7 +45,7 @@ def write_json(jsonout):
>>  def set_json_mode():
>>      global UI_mode
>>      UI_mode = 'json'
>> -    jsonout = {'dialog': 'apparmor-json-version', 'data': '2.12'}
>> +    jsonout = {'dialog': 'apparmor-json-version', 'data': '2.13'}
>>      write_json(jsonout)

Based on today's discussion. Should this version be any different?
We do have a catchall for invalid dialog in yast, so we may not require
it after all.

>>
>>  # reads the response on command line for json and verifies the
>> response @@ -250,9 +250,16 @@ def
>> generate_diff_with_comments(oldprofile, newprofile): def
>> UI_Changes(oldprofile, newprofile, comments=False):
>>      if comments == False:
>>        difftemp = generate_diff(oldprofile, newprofile)
>> +      header = 'View Changes'
>>      else:
>>        difftemp = generate_diff_with_comments(oldprofile, newprofile)
>> -    subprocess.call('less %s' % difftemp.name, shell=True)
>> +      header = 'View Changes with comments'
>> +    if UI_mode == 'json':
>> +        jsonout = {'dialog': 'changes', 'header':header, 'filename':
>> difftemp.name} +        write_json(jsonout)
>> +        response = json_response('changes')["response"]
> 
> make check  complains:
>     apparmor/ui.py:260: local variable 'response' is assigned to but never used
> 
> Obviously nobody cares about the response of displaying the diff (but
> expecting a response is still a good idea to keep the interface 
> consistent). Therefore I'll change this line to
> 
> +        json_response('changes')["response"]  # response gets ignored, therefore not assigning to a variable

I won't say it is consistent, because apparmor-json-version does not
require a response.

Yes, thats how it should be. I added a response primarily so that the
temporary file does not get deleted on closure. Perhaps you may want to
add that in the comment.

> 
>> +    else:
>> +      subprocess.call('less %s' % difftemp.name, shell=True)
>>      difftemp.close()
>>
>>  CMDS = {'CMD_ALLOW': _('(A)llow'),
> 
> With the above change:
>     Acked-by: Christian Boltz <apparmor at cboltz.de>
> 
> 
> Regards,
> 
> Christian Boltz
> 
> 
> 

-- 
Goldwyn