Hi Nirmal-
Here's some feedback....
Thanks!
/jb
ai_get_manifest.py
line 381:
Nit - cmd_op sounds a bit like an opcode or something, maybe just use
"output" or similar? Also, that could be my background, which is why it's a nit
- feel free to ignore if you want :)
line 384:
Nit - You could use splitlines() rather than split"\n")
line 385:
If something goes wrong here wrt the output, or if the format changes
and you for whatever reason find yourself with a line without a ":" or with at
least the right number of elements (in this case, three), this will dump a
stack. Maybe put it in a try/except ValueError. Or, possibly just validate the
string?
line 426, 467:
Same comment as with 385
line 509:
nit - There are likely cleaner ways to do this. Given we have about 8
different places in the code where we get a netmask, it might be a good idea to
see if we can put this in one place and call it. Maybe a low-priority CR for
this would be good?
installadm-common.sh
line 893:
looks like maybe an indent problem here?
On Apr 5, 2012, at 11:05 AM, Nirmal Agarwal wrote:
> Hi all
>
> A gentle reminder, I would appreciate if someone can review this fix.
>
>
> Thanks
> Nirmal
>
>
> On 4/3/2012 5:28 PM, Nirmal Agarwal wrote:
>> Hi all
>>
>> Can I please get a code review for CR 6996539.
>>
>> 6996539 Convert AI to use ipadm instead of ifconfig
>>
>>
>> Webrev :
>> https://cr.opensolaris.org/action/browse/caiman/nirmal27/6996539/webrev/
>>
>> I have removed get_ip_netmask() and find_network_nmask() from
>> installadm-common.sh. I couldn't find reference to them in the gate.
>> Let me know if I missed something.
>>
>> Pep8 is clean and pylint output is unchanged.
>>
>> Testing :
>>
>> -> Created custom image and tested client boots and sets the
>> criteria correctly.
>>
>> --> installed the installadm packages from nightly and created
>> a service.
>>
>> Let me know if I need to run some other tests.
>>
>> Thanks
>> Nirmal
>> _______________________________________________
>> caiman-discuss mailing list
>> [email protected]
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
> _______________________________________________
> caiman-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
