On 06/03/11 14:08, Sue Sohn wrote:
On 06/ 2/11 03:28 PM, Jack Schwartz wrote:
Hi everyone.

Here is a webrev of the installadm manpage changes relevant to the Derived
Manifests project.

Changes to installadm.1m also include some quick reformatting (so the manpage prints better). However, reformatting is not complete since Sue will be doing an
overhaul with the ISIM project soon.

Once it is reviewed, I will push it to slim_source.

Also included, as an extra bonus :) is the aimanifest manpage. This still in progress but I'm close to being done with it. Please have a look and let me know if you have comments. I plan on giving this to Alta to work her magic on it
before pushing it to the gate, since I'm the only one working on it.

http://cr.opensolaris.org/~schwartz/110602.1/webrev/index.html

Please submit any comments by Tuesday 6/7 COB.

Thanks for your time,
Jack



_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

Hi Jack,
I just looked at the installadm man page.

General: all references to "install service" should say "installation service"

16 Remove '...'
OK.  Only one prop at a time...

29 Change:
<manifest/script_file> -> <manifest/script file>
or
<manifest/script_file> -> <manifest_file/script_file>
(to get across the idea that the manifest is a file)
OK.
Changed all manifest/script_file -> manifest/script file
Changed all manifest/script_name -> manifest/script name

35 Please add --options, such as -n|--service <svcname>.
This would apply to the other commands, but as I've already added them in the isim gate, you don't really need to do it here.

64 Need to add the -o option for export:
     -o|--output <pathname>
Thanks.  Fixed.

287 remove '...'
Fixed.

289-292 How about simply:
     Sets a property of a service to a specified value.
I'm not exactly sure of the intended meaning of the second sentence, but I doubt it would apply to all properties.

This is what I had (minus the alias portions)- you're welcome to use all or parts of it.
     installadm set-service -o <prop>=<value> <svcname>

         Sets a property of a service to a specified value.

         -o|--option <prop>=<value>
            Specifies the property and value to set.

            prop=value can be:
                 default-manifest=<manifest/script name>

            - default-manifest=<manifest/script name>
              The default manifest of a service may not have
              criteria actively associated with it while it is
              the default. If one uses installadm set-service
              to make a manifest the default, then its
              associated criteria become "inactive", and are
              not considered during manifest selection.
              Inactive criteria are clearly indicated by the
              installadm list command. If a different manifest
              is later made the default, the criteria of the
              formerly default manifest becomes active again.

<svcname>
              Required: Specifies the name of the installation
              service whose property is being set.
I changed all but the big paragraph for this subcommand. However, I added a paragraph in the DESCRIPTION section explaining what a default manifest or script is. Why explain repeatedly what a default is, when explaining it once in the description section is most succinct and clear, and all that's needed?
346-358 If you are going to add this level of detail for -m, you should also add it for -p.
OK.  Added the following to -p:

         When -n is not specified, display an abbreviated
         listing per service that includes the profile names.

         When -n is specified, display the profiles for the
         requested service along with their criteria.

424 Change inst_name to man_name
OK

433 refered (typo)
Fixed.

443 not your changes, but please remove extra '-c' inside of <>
Actually the command would imply one could specify many crit=value pairs with a single -c, which isn't true. I removed the extra set of <> as well as the extra -c, and added that "A command can specify -c multiple times." I did this for set-criteria, add-manifest and create-profile subcommands.

Processing this change pointed out an issue where if someone happens to put multiple criteria in quotes:
    ... -c "arch=x86 mem=2048"
installadm treats this as arch = "x86 mem=2048"
or a single criteria "arch" with a value of "x86 mem=2048"
which may be confusing to people...

461 Use "ignored" instead of "inactive", since that is how it is described elsewhere and in the list output
Thanks.  Fixed.

471-472 How about: Any associated criteria or default status remain with the manifest following the update.
Yes, that is clearer.  Thanks.

474-483 I don't understand this wrt how update-manifest works. I thought that update-manifest updates the contents of a given manifest, specified by the required -m option. I didn't think the name would change. We discussed this offline and you're looking into it, but I think this section should probably be removed and -m should be required (small code change)
OK, I think things are OK codewise, but I needed the change the manpage. This command will *not* change the ai_instance/name attribute of an existing manifest, but will only update the contents, which is what is it supposed to do. The manpage now reads as follows:

          The name of the manifest or script to be updated
          is derived from (in order):
          1) installadm update-manifest -m option, if
             present
          2) manifest name attribute, if present in the
             manifest and it matches one in an existing
             manifest, i.e.
<ai_instance name="inst_name">
          3) manifest/script file name, if it matches the
             manifest name attribute in an existing manifest,
             or the name given by installadm list if it matches
             the name of an existing script.
495 per above, this should likely be Required, not Optional

622 usage should match actual usage (different order of options, usage has <output>, not <output_pathname> )
I'll change the usage, as output_pathname is clearer.

642 <output_pathname> again

636 It is confusing to mention profiles as part of -m text and to mention manifests as part of line 652 text. Maybe you can remove the "Any number of..." text from both -m and -p and add it the general description.
I simplified both -m and -p descriptions by removing the "Any number of..." text and saying that "A command can specify -m (or -p) multiple times." The command's description already said that "At least one manifest/script or profile must be provided."
646 need space after comma
Fixed.

963 - Check with Alta if this is the name to use.
Changed to "Installing Oracle Solaris 11 Systems" per Alta.

971 your installadm add-manifest command is missing the "-n service_092910"
OK.  Changed to:
(using installadm add-manifest -m my_manifest -n service_092910 ...)

The idea is to show only the relevant parts of the command to keep it simple.

1182 stored in the file -> stored in the files
Fixed.

1206 install the changes -> then update the manifest in svc2
OK.

1211 remove "with you favorite editor"
OK.

1214 Remove backslash
Thanks.

I'll repost at the end of the code review.

    Thanks again for your review.

    Jack

Sue

_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

Reply via email to