On 11/23/2015 01:50 PM, Jan Cholasta wrote:
> On 23.11.2015 13:40, Tomas Babej wrote:
>>
>>
>> On 11/23/2015 01:31 PM, Jan Cholasta wrote:
>>> On 23.11.2015 13:28, Tomas Babej wrote:
>>>>
>>>>
>>>> On 11/23/2015 01:11 PM, Jan Cholasta wrote:
>>>>> On 23.11.2015 12:53, Tomas Babej wrote:
>>>>>> Hi,
>>>>>>
>>>>>> If the code within the private_ccache contextmanager does not
>>>>>> set/removes the KRB5CCNAME, the pop method will raise KeyError, which
>>>>>> will cause unnecessary termination of the code flow.
>>>>>>
>>>>>> Make sure the KRB5CCNAME is popped out of os.environ only if present.
>>>>>>
>>>>>> Tomas
>>>>>
>>>>> NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.
>>>>
>>>> Yeah, well, both ways are equivalent, I found the if statement more
>>>> explicit. We can go with the suggested version, if it's more pleasing
>>>> though - patch is attached.
>>>>
>>>>>
>>>>> Also I don't think the comment is necessary, it's quite obvious
>>>>> what the
>>>>> code does.
>>>>>
>>>>
>>>> I don't think an explanatory comment can hurt, ever. Worst thing that
>>>> happens is that somebody is assured that he understands the code
>>>> correctly.
>>>
>>> Actually the worst thing is that someone changes the code without
>>> changing the comment. If the comment does not add any real value, it's
>>> only a maintenance burden.
>>>
>>
>> Yep, that's a real issue in our code base (i.e. I recently removed such
>> a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
>> sure the comments describe the implementation properly is on the
>> author/reviewer though.
>>
>> What's "added value" highly depends on your skill set, and familiarity
>> with the code base. Particularly the familiarity with the code base can
>> diminish over time even for the author, and those are the times where
>> comments can come to the rescue.
>
> Let's take a look at the code:
>
> if original_value is not None:
> os.environ['KRB5CCNAME'] = original_value
> else:
> os.environ.pop('KRB5CCNAME', None)
>
> Can you tell me what in there couldn't be obvious to a person with even
> the most basic skill set?
>
> IMHO a docstring for private_cccache would add some real value, but this
> comment alone does not.
>
>>
>> For a newcomer to the project, even a trivial comment (from the point of
>> view of the experienced developer) can be valuable.
>
> Following this logic, there should be a comment for every line of our
> code, which is ridiculous.
>
Here's the version of the patch with the comment removed.
Tomas
From f87ae19e673d4fe2ae1330fb2b1e1a1dbaa55630 Mon Sep 17 00:00:00 2001
From: Tomas Babej <[email protected]>
Date: Mon, 23 Nov 2015 12:47:56 +0100
Subject: [PATCH] private_ccache: Harden the removal of KRB5CCNAME env variable
If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.
Make sure the KRB5CCNAME is popped out of os.environ only if present.
---
ipapython/ipautil.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..a5545688d61354716745c7814b6bbf1ae316c13d 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1367,7 +1367,7 @@ def private_ccache(path=None):
if original_value is not None:
os.environ['KRB5CCNAME'] = original_value
else:
- os.environ.pop('KRB5CCNAME')
+ os.environ.pop('KRB5CCNAME', None)
if os.path.exists(path):
os.remove(path)
--
2.5.0
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
