Hi Clay.
All is well except one nit:
On 10/06/09 21:42, Clay Baenziger wrote:
> Hi Jack,
> Thank you for talking with me on the phone today.
>
> For the comment on line 425, I've changed the comment to:
> # remove line containing boot archive (updates /etc/vfstab)
>
> For the comment on lines 685-687, I've used the comment:
> # No SMF properties found, nor files to identify this arch as
> # SPARC; so, try looking for X86 files.
My understanding is that SMF properties are used to identify SPARC, but
the comment doesn't make that connection. If I am correct, it's more
acccurate to say:
No SMF properties nor files found to identify this arch as SPARC.
I hope you decide to change it; no need for another review.
Thanks,
Jack
> # If /tftpboot/<service_name> exists, we know it's X86 architecture.
>
> Lastly, I agree about needing to pull out fixed strings, but I think a
> number of these should be stored in SMF per:
> 11052 - create-service: should leave record of microroot in SMF for x86
> services And otherwise a common solution should be implemented per
> bug:
> 4402 - Pull fixed strings in A/I server python code out to a
> separate module
>
> And as you pointed out ICT or DC has picked a common method which
> should be looked at for reuse (where a module takes a C header file
> and transforms it to a number of Python strings).
> Thank you,
> Clay
>
> On Fri, 2 Oct 2009, Jack Schwartz wrote:
>
>> Hi Clay.
>>
>> I'm OK with the items I've snipped; comments below on other items.
>>
>> On 10/02/09 14:28, Clay Baenziger wrote:
>>>
>>>> 380, 397: /var/ai/image-server/images is in these two places at
>>>> least. Please define a common string somewhere for these. Better
>>>> yet, /var/ai is used in even more places. If the string
>>>> /var/ai/image-server/images would have to change if the /var/ai
>>>> string did, please define /var/ai somewhere and use in all places
>>>> the variable which carries that string.
>>>
>>> I agree, this is bug 4402 (Pull fixed strings in A/I server python
>>> code out to a separate module) as I'd like to look into some various
>>> solutions for this. I don't think I've yet found a clean way to do
>>> this as I'd like.
>> Well, OK, but the longer we wait to fix this kind of thing, the more
>> unmaintainable the code gets in the meantime...
>>>
>>>> 425: Does the microroot-less vfstab file get written somehow?
>>>
>>> The function on 418 updates the backing-store (the /etc/vfstab file):
>>> del(vfstabObj.fields.MOUNT_POINT[idx])
>> OK. I found this in installadm_common.py line 425, but is not
>> obvious. Please add a comment that this happens.
>>>> 685: I don't understand the comments here. I think "again" may be
>>>> causing confusion. Also, what SMF tests were run? Can 686/687 be
>>>> a separate comment from 685?
>>>
>>> Good point this was rather vague. How's this for clarification?
>>> # no identifying SMF properties were found, nor were SPARC files found,
>>> # so try looking for X86 files.
>>> # both X86 clients and services need equivalent files removed
>>> # from tftpboot (so, look for /tftpboot/<service or client name>)
>> This is still not clear to me. Not sure how the second part about
>> X86 clients and services need equivalent files removed is relevant
>> here. As for the first part, is this accurate:
>>
>> No SMF properties or files were found to identify this arch as SPARC,
>> so try looking for X86 files.
>>
>> Thanks,
>> Jack
>>
>>
