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 >> > >
