Jack,
You have been busy. You've got a bunch of good work here!!
Comments below. I do not need to see another code review
for these comments.
Thanks,
John
================================================================================
usr/src/cmd/Makefile
================================================================================
1. Line 34 should start with a tab and not 4 spaces.
33 PYTHONSUBDIRS= aimanifest ai-webserver distro_const installadm js2ai \
34 text-install
================================================================================
usr/src/cmd/ai-webserver/README.test
================================================================================
1. I think that there is an assumption that this test will be run on a
system
without a dummy54123 service. Is that correct? If so then perhaps that
should be spelled out.
================================================================================
usr/src/cmd/ai-webserver/manual-test/man_test_default.py
================================================================================
1. manifest_name does not need to be part of the class space. Thus if
line 55
were to be removed and the self. prefix could also be removed from lines
100, 102, 103, 104 and 105. Though this is not part of your code
changes.
55 manifest_name = None
100 self.manifest_name = "temp_mfest_name_%d" % os.getpid()
102 properties.set_default(self.service_name, self.manifest_name,
103 properties.SKIP_MANIFEST_CHECK)
104 self.assertEqual(self.manifest_name,
105 properties.get_default(self.service_name))
================================================================================
usr/src/cmd/installadm/ai_smf_service.py
================================================================================
Looks fine.
================================================================================
usr/src/cmd/installadm/svc-install-server
================================================================================
1. The following should probably state that it is attempting to upgrade.
210 echo "Upgrading service ${service_name} to " \
211 "version ${AI_SERVICE_VERSION}"
Then at the end the script should say that the service was upgraded.
Perhaps this should also be moved lower within the script. If the
default-manifest property already exists then it would seem that the
service does not need upgrading.
2. This should be moved down to a place where you are sure that the upgrade
took place. Prehaps after line 244.
213 # Install version for this AI service
214 ${SVCCFG} -s ${SMF_FMRI} setprop
${service}/version = \
215 astring: \"${AI_SERVICE_VERSION}\"
3. The following should be changed:
223 echo "Checking whether mfest_dir
$mfest_dir exists..."
Perhaps something to the effect of "Checking the service directory
$mfest_dir".
4. The following should be changed:
225 echo "Nope... Getting port"
Perhaps instead mentioning that the service is using port compatibility
instead of the service directory.
5. Then state that it is using the port compatibility directory instead of:
230 echo "Port = $port"
These other output lines seem like they are personal debugging comments
instead of production output.
6. The following is not needed:
238 echo "name of default file is ${default_file}"
7. This seems to need to be changed too:
240 echo "default_file found; running
installadm"
Perhaps something to the effect "default_file found; Upgrading
Solaris Automated
Install Service ${service} to version ${version}" or some such.
================================================================================
usr/src/lib/Makefile
================================================================================
Looks fine.
================================================================================
usr/src/lib/Makefile.targ
================================================================================
Looks fine.
================================================================================
usr/src/lib/install_manifest_input/mim.py
================================================================================
Looks fine.
================================================================================
usr/src/lib/install_manifest_input/test/test_manifest_input_set_get_add.py
================================================================================
Did not review thoroughly. Though skimmed and looks fine.
================================================================================
usr/src/pkg/manifests/system-install-auto-install.mf
================================================================================
Looks good.
================================================================================
usr/src/tools/tests/tests.nose
================================================================================
Looks good.
================================================================================
On 04/29/11 01:21 PM, Jack Schwartz wrote:
Hi everyone, esp Ethan, John and Keith.
Per discussion with Ethan, John and Keith, I have updated the
svc-install-server SMF script with the following changes:
1) Rename smf_default_manifest_upgrade() to upgrade_svc_vers_0_to_1()
and always call it at startup time.
2) Remove the all_services/version property. (Note: server.xml is now
removed as well, as this property was defined in that file.)
3) Have upgrade_svc_ver_0_to_1() check each AI service to see if it is
at the current version, and upgrade it if required.
I've built packages, and retested the script.
I'll push as soon as one of you bless.
Delta:
http://cr.opensolaris.org/~schwartz/110428.1/webrev.2.3.diff/index.html
vs slim_source:
http://cr.opensolaris.org/~schwartz/110428.1/webrev/index.html
Thanks,
Jack
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
