Jesse,
Lots of text changes = lots of good work done. Keep up the good work.
Below are my comments.
Thanks,
John
================================================================================
usr/src/cmd/installadm/ai_smf_service.py
================================================================================
1. Should the following text be wrapped with a cw() function call?
85 raise ServicesError(_("The system does not have the "
86 "system/install/server SMF service.\n"))
================================================================================
usr/src/cmd/installadm/aimdns_mod.py
================================================================================
fine
================================================================================
usr/src/cmd/installadm/aimdnsd.py
================================================================================
fine
================================================================================
usr/src/cmd/installadm/client_control.py
================================================================================
fine
================================================================================
usr/src/cmd/installadm/create_client.py
================================================================================
1. cw imported but not used. It looks like cw() should be used for
lines 110
and 157. Perhaps the parser.error() lines as well.
================================================================================
usr/src/cmd/installadm/create_service.py
================================================================================
1. Unsure why lines 195-197 are cw() wrapped and others lines like
201-202 are
not:
195 parser.error(cw(_('\nDHCP server configuration is
unavailable on '
196 'hosts with multiple network
interfaces (-i and '
197 '-c options are disallowed).\n')))
201 parser.error(_('\nIf -i option is provided, -c option
must '
202 'also be provided\n'))
================================================================================
usr/src/cmd/installadm/delete_client.py
================================================================================
fine.
================================================================================
usr/src/cmd/installadm/delete_service.py
================================================================================
1. Is redirecting print statements to sys.stdout really necessary?
114 print >> sys.stdout, cw(_("Removing this service's
bootfile "
115 "from local DHCP
configuration\n"))
194 print >> sys.stdout, cw(_(_warning))
If so then we should be consistent and line 136-138 should also be
redirected.
2. Should the following be cw() wrapped:
195 prompt = _("Are you sure you want to delete this
alias? [y/N]: ")
================================================================================
usr/src/cmd/installadm/dhcp.py
================================================================================
1. Perhaps another cw() wrapping needed:
662 raise ValueError(_("unsupported action on DHCPServer
object: %s")
663 % action)
881 raise DHCPServerError(_("host [%s] already present in
the DHCP "
882 "configuration") % macaddr)
================================================================================
usr/src/cmd/installadm/image.py
================================================================================
fine.
================================================================================
usr/src/cmd/installadm/installadm-convert.py
================================================================================
1. Uses cw() wrapper but does not appear to import it.
2. Does the following need a cw() wrapper:
1555 print _(' No default manifest found for service %s' %
ai_service)
================================================================================
usr/src/cmd/installadm/installadm.py
================================================================================
fine.
================================================================================
usr/src/cmd/installadm/installadm_common.py
================================================================================
fine.
================================================================================
usr/src/cmd/installadm/list.py
================================================================================
fine.
================================================================================
usr/src/cmd/installadm/rename_service.py
================================================================================
fine.
================================================================================
usr/src/cmd/installadm/service.py
================================================================================
1. Why not use a single quote (') around the text string instead of a double
quote (") as this would get away from the need to use the backslash
escape
(\)?
274 raise UnsupportedAliasError(cw(_("\nCannot create
alias: Aliased "
275 "service \"%s\" does
not support "
276 "aliasing. Please use
a service "
277 "with a newer
image.\n") % alias))
767 raise UnsupportedAliasError(cw(_("\nCannot update
alias: Aliased "
768 "service \"%s\" does
not support "
769 "aliasing. Please use
a service "
770 "with a newer
image.\n") %
771 newbasesvc_name))
Alternatively, you could use a single quote (') around the %s like is
done
below:
843 error = cw(_("\nManifest '%s' is not valid for
this image. "
844 "Please fix and update this
manifest.\n") %
845 manifest)
896 error = cw(_("\nProfile '%s' is not valid for this
image. "
897 "Please fix and update this
profile.\n") %
898 profile_name)
Either way these strings should be treated constistently.
2. Perhaps quotes are needed around the %s:
1159 raise ValueError(_("\nManifest %s does not exist.\n") %
1160 manifest_path)
================================================================================
usr/src/cmd/installadm/service_config.py
================================================================================
fine.
================================================================================
usr/src/cmd/installadm/set_service.py
================================================================================
fine.
On 07/14/11 10:39 AM, Jesse Butler wrote:
Could I please get a review for my changeset which addresses:
7059977 some installadm output needs better formatting
7061072 installadm should inform of DHCP configuration steps
For the first CR, I've implemented a new common route to use throughout
installadm, cli_wrap(). It uses the textwrap module's dedent and fill routines
to ensure leading whitespace is whacked out and then all following text is
wrapped at X characters. I've chosen 70 to wrap at, and I think the text blocks
look good. I've also added some whitespace in some places where the output gets
a bit loud, so that things look a bit cleaner.
For the second CR I've added some explicit statements during the optional local
DHCP configuration changes, specifically when creating a new service, or adding
or deleting a client binding. I've also added logging statements for the DHCP
code as we were lacking that.
https://cr.opensolaris.org/action/browse/caiman/jesseb/output_cleanup/webrev/
Please see below for a sampling of the new outputs.
Thanks very much,
Jesse
===============
create-service and delete-service, w/ local DHCP configuration
------------------------------------------------------------------------------------
# installadm create-service -n i386_snv169 -s
/var/tmp/images/i386/sol-11-dev-169-ai-x86.iso -d
/install/images/i386_snv169 -i 10.80.238.150 -c 20
Creating service: i386_snv169
Setting up the target image at /install/images/i386_snv169 ...
Starting DHCP server... !! only seen when setting up new DHCP
Adding IP range to local DHCP configuration
Setting the i386 bootfile in the local DHCP configuration
Refreshing install services
#
-----
# installadm delete-service default-i386
WARNING: The service you are deleting, or a dependent alias, is
the alias for the default i386 service. Without the 'default-i386'
service, x86 clients will fail to boot unless explicitly
assigned to a service using the create-client command.
Are you sure you want to delete this alias? [y/N]: y
Removing this service's bootfile from local DHCP configuration
Stopping the service default-i386
#
-----
# installadm delete-service i386_snv169
#<no output>
w/o local DHCP:
----------------------
# installadm create-service -n i386_snv169 -s
/var/tmp/images/i386/sol-11-dev-169-ai-x86.iso -d /install/images/i386_snv169
Creating service: i386_snv169
Setting up the target image at /install/images/i386_snv169 ...
No local DHCP configuration found. The DHCP server may enable this
service as a default by referencing its bootfile, if not already
enabled. Client-specific bindings may also be created via the
'create- client' subcommand. See installadm(1M) for further information.
Refreshing install services
#
-----
# installadm delete-service default-i386
WARNING: The service you are deleting, or a dependent alias, is
the alias for the default i386 service. Without the 'default-i386'
service, x86 clients will fail to boot unless explicitly
assigned to a service using the create-client command.
Are you sure you want to delete this alias? [y/N]: y
No local DHCP configuration found. Ensure that the bootfile for this
service is no longer referenced by the DHCP server in use.
#
-----
# installadm delete-service i386_snv169
No local DHCP configuration found. Ensure that the bootfile for this
service is no longer referenced by the DHCP server in use.
#
create-client and delete-client, w/ local DHCP present:
-------------------------------------------------------------------------
# installadm create-client -e 00:11:2:3:44:5 -n i386_snv169
Adding host entry for 00:11:02:03:44:05 to local DHCP configuration.
#
-----
# installadm delete-client 00:11:2:3:44:5
Removing host entry for 00:11:02:03:44:05 from local DHCP
configuration.
#
w/o local DHCP present:
---------------------------------
# installadm create-client -e 00:11:22:33:44:55 -n i386_snv169
No local DHCP configuration found. The DHCP server should set bootfile
'01001122334455' for this client.
#
-----
# installadm delete-client 00:11:22:33:44:55
No local DHCP configuration found. Ensure the DHCP server no longer
references the bootfile for this client.
#
_______________________________________________
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
