On 04/29/11 02:24 PM, John Fischer wrote:
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
These lines are now of the same form as usr/src/lib/Makefile
================================================================================
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.
Done.
================================================================================
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))
Fixed.
================================================================================
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.
I'd rather not add more verbage. There's already a lot. If the
default-manifest property should be missing only for versions less than
1 anyway.
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}\"
OK.
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".
Changed to "Checking whether manifest directory $mfest_dir exists."
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.
Changed to "No... Checking port compatibility directory instead..."
5. Then state that it is using the port compatibility directory
instead of:
230 echo "Port = $port"
Just removed this line.
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}"
Actually I want to keep it as it shows which dir was selected. changed to:
"Looking for default file ${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.
Changed to "Default file found. Upgrading service."
I also added a test for installadm failure and do the right thing.
================================================================================
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.
================================================================================
Thanks for reviewing.
Final webrev is in the same place as before.
I'm going to push.
Thanks,
Jack
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
