Hi Jan.
Sure. Leave the bug ID in and push.
Years ago when working on the USB team, I wanted to leave a bug ID in a
comment for the same reasons as Dave M, David C and you have elaborated
but was told not to. However, that was a different time and I was
working with a different set of people.
Thanks,
Jack
On 04/29/09 01:36, Jan Damborsky wrote:
> Hi Jack,
>
>
> Jack Schwartz wrote:
>> Hi Jan.
>>
>> Thanks for making those changes.
>>
>> One other nit was introduced this iteration though...
>>
>> My understanding is that it is bad practice to put bug IDs into code
>> comments. This was done on line 885 of ict.py regarding the dumpadm
>> -r. Please remove the reference to 6835106 and file another bug to
>> document what needs to be fixed after 6835106 is fixed.
>
> I tend to agree with Dave in this point. In such cases, referencing
> bug number might save time and is only temporary - it is assumed
> that once related bug is fixed, workaround can be removed and comment
> modified appropriately. Thinking about this particular scenario, I had
> to go through the following process:
>
> Seeing in comments that I can't use 'dumpadm(1M) -r' and need
> to manipulate dumpadm.conf file directly instead, I had to
>
> [1] investigate why I can't use it
> [2] find out if the problem still exists
> [3] search if bug was already filed for this issue
>
> If bug had been already filed for this along with bug number
> mentioned in comments, I would have just taken a look at
> bug in order to determine if workaround was still needed or
> could be removed.
> In case was fixed, then workaround would be removed and comment
> changed appropriately.
> Based on this, I would rather leave reference to the bug number
> in comments.
>
> Please let me know what you think.
>
> Thank you,
> Jan
>
>>
>> No need to send me another review for this change.
>>
>> Thanks,
>> Jack
>>
>> Jan Damborsky wrote:
>>> Jack Schwartz wrote:
>>>> Hi Jan.
>>>>
>>>> I reviewed your code changes and they look reasonable to me.
>>>>
>>>> - I looked at the order that the new entries for
>>>> update_dumpadm_nodename() are called in install_finish, which seems
>>>> to be correct when I look at the existing call there for the GUI
>>>> install.
>>>>
>>>> - I'm OK with just pulling out the extra step of a temporary file
>>>> used to create dumpadm.conf (since this is a new system anyway and
>>>> dumps anyway would be critical to be able to use the system).
>>>>
>>>> My only nit is ict.py / lines 889- 890. Since you're copying
>>>> dumpadm.conf from a running system to another BASEDIR, I would
>>>> suggest something like this:
>>>>
>>>> dumpadmfile = '/etc/dumpadm.conf'
>>>> dumpadmfile_dest = self.BASEDIR + dumpadmfile
>>>>
>>>> ... but this is only a suggestion.
>>>
>>> Hi Jack,
>>>
>>> This is a good point. I have changed this according
>>> to your suggestion and updated the webrev.
>>>
>>> Thank you very much for reviewing those changes,
>>> Jan
>>>
>>
>>
>
