Hi Harold,
delete_service needs updated copyright. Otherwise, looks good now.
Sue
p.s. I recommend filing doc bugs now. Helps with getting the rti approved.
On 01/ 4/12 02:21 PM, Harold Shaw wrote:
Hi Sue,
Thanks again for the review. I have made the following changes in addition to
those already listed
below:
- found and fixed 3 instances of the architecture value (i386, sparc) being
translated and printed
as (x86, SPARC) (2 create_service.py, 1 delete_service.py)
- Made all of the error messages in instantiations of ImageError consistent in
style.
Additional tests:
- Ran a test on delete-service to verify that the changed error message printed
out correctly.
Updated webrev:
https://cr.opensolaris.org/action/browse/caiman/hshaw/5393_2
I will file the docs/man page bug(s) as soon as I putback.
Thanks,
Harold
On 01/04/12 10:24, Sue Sohn wrote:
On 01/ 4/12 08:17 AM, Harold Shaw wrote:
On 01/03/12 16:54, Sue Sohn wrote:
On 01/ 3/12 02:06 PM, Harold Shaw wrote:
May I get a re-review for the following installadm bugs:
7060060 Informational message to report bad image should include service name
5393 Automated Installer should not allow setting up non-AI images and should
check validity of
image
7090169 installadm list doesn't detect when the image directory has been removed
I changed the output for 'installadm list' and 'installadm list -c' as follows
for the case where
the service image directory is not available to look up the architecture.
Everything lines up when
printed to the screen.
root@x2270-brm-03:/export# installadm list -c
Service Name Client Address Arch Image Path
------------ -------------- ---- ----------
s11u1_iso_2 01:02:03:04:05:06 * /export/iso_2
* - Architecture unknown, service image does not exist or is
inaccessible.
root@x2270-brm-03:/export# installadm list
Service Name Alias Of Status Arch Image Path
------------ -------- ------ ---- ----------
default-i386 s11u1_pkg on * /export/auto_install/s11u1_pkg
s11u1_iso - on i386 /export/auto_install/s11u1_iso
s11u1_iso_2 - on * /export/iso_2
s11u1_pkg - on * /export/auto_install/s11u1_pkg
solaris11u1-i386-05 - on i386 /export/auto_install/solaris11u1-i386-05
solaris11u1-i386-06 - on i386 /export/auto_install/solaris11u1-i386-06
* - Architecture unknown, service image does not exist or is
inaccessible.
Webrev:
https://cr.opensolaris.org/action/browse/caiman/hshaw/5393_1
Testing:
Create an AI service from a valid ISO
Create an AI service from a valid package
Attempt to create AI service from ISO with:
- no solaris.zlib
- no auto_install/ai.dtd
This was done by attempting to create a service from a text install iso and a
hand modified iso
(to
get rid of solaris.zlib).
Disable and enable valid services
Disable valid services, make them invalid (remove the image dir, get rid of
auto_install or
solaris.zlib), and then attempt to enable them
installadm list & installadm list -c
remove image directory, run installadm list, and verify that it runs
successfully printing the
Arch
value as '*'
Created service image packages (using pkgrecv & pkgmogrify) with:
- no solaris.zlib
- no auto_install/ai.dtd
and attempted to create services from them. This was to verify that the
original error message
would
still be printed.
Hi Harold,
Happy New Year. Please update copyright to 2012 for all files. :)
Done.
image.py
225 change "autoinstall image" to "Automated Installer image" to be consistent
with line 78
Not really part of my changes but I agree that it should be done.
list.py
119 Previously, which_arch() returned 'x86' or 'Sparc' and now it returns (and
outputs) 'i386' and
'sparc'. Is this intentional? If so, manpage examples and docs will need update.
There is an inconsistency in usage that I was attempting to correct. I didn't
consider the docs
implications. Thanks for pointing that out.
The problem is that installadm currently reports arch differently for
'installadm list' and
'installadm list -c' since they have different methods of obtaining the string
values.
The following excerpts are from the installadm man page:
$ installadm list
Service Name Alias Of Status Arch Image Path
------------ -------- ------ ---- ----------
default-i386 sol-11-i386-svc on x86 /export/images/soli386
default-sparc sol-11-sparc-svc on Sparc /export/images/solsparc
sol-11-i386-svc - on x86 /export/images/soli386
sol-11-sparc-svc - on Sparc /export/images/solsparc
$ installadm list -c -n my-x86-service
Service Name Client Address Arch Image Path
------------ -------------- ---- ----------
my-x86-service 01:C2:52:E6:4B:E1 i386 /export/images/myimage
The legal values for arch in create-service are 'i386' and 'sparc'. It seems we
should be reporting
back the same values as specified when creating the service.
We have 2 courses of action.
1. Leave the changes and file docs bugs to correct the example output in the
man page and docs.
2. Revert this part of the change back to the original usage. This method
requires no changes to the
docs.
My preference would be option 1.
I also prefer option 1, but just wanted to make sure that it was a purposeful
change and that you
were aware of doc impact.
Also, I know of at least one place in create-service which prints out the
architecture of the
service being created as x86, so to be consistent, you should change that as
well.
129-130 comments need to be updated
184 and 478 add comma after "identified"
191 and 494 Suggest defining this string at the top of the file since it is
used in a couple of
places
Done.
service.py
682 add-> prepend
Done.
685 Now that you have the service name first, you should remove the tab before
"Service Name"
Also, it looks like the ImageError strings may already start with a newline, so
you may not want
the newline after the service name
I think a better approach would be to get rid of the newline at the start of
the ImageError strings.
There are several invocations of ImageError and only 2 begin with a newline.
Or maybe consider adding newlines to the other ones...your discretion.
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
