[caiman-discuss] CR request for doo
Hi Ethan,
If we want we can store what's exactly stored in the XML AI
manifest name tag in the AI database and then use any arbitrary file name
we please. The thought was for an administrator to (if they wanted) be
able to go to http://:/manifests/.xml to get at the manifest. This is what's available today.
Thank you,
Clay
On Fri, 5 Mar 2010, Ethan Quach wrote:
> Hi Clay,
>
> On 03/04/10 08:53, Clay Baenziger wrote:
>> Hi Ethan,
>> We already ensure all manifest files end in .xml[1]. Also,
>> delete-manifest already checks if the manifest requested is not found in
>> the AI DB, if it's listed ending with .xml[2].
>
> I'm not questioning if the current implemented code already
> does these checks. My comment is really asking if we should
> relook at that to see if there is a better approach.
>
> What I'm seeing here (and correct me if I'm wrong) is that we
> take a user provided , and base an internal
> filename from that. But the internal filename is generated
> conditionally. Sometimes it'll end up being
>
> .xml
>
> and other times its just
>
>
>
> Then we have subsequent code chase that conditional when
> something needs get back out that user because
> we have to extrapolate it from the internal filename.
>
> Is there perhaps a better (more direct) way for that subsequent
> code to get at ?
>
>
>> I think your pathological case is valid as if someone names their AI
>> manifest "foo.xml.xml" it will be stored that way but the admin. would have
>> to have some bizarre reason.
>
> The reasons are irrelevant. Its a 'text string' value, and the user
> has no notion, nor should they be impacted by, what we do with
> it internally.
>
>
> thanks,
> -ethan
>
>>
>> Thank you,
>> Clay
>>
>> [1]: publish-manifest
>> http://src.opensolaris.org/source/xref/caiman/slim_bp_installgrub/usr/src/cmd/ai-webserver/publish-manifest.py#692
>>
>>
>> [2]: delete-manifest
>> http://src.opensolaris.org/source/xref/caiman/slim_bp_installgrub/usr/src/cmd/ai-webserver/delete-manifest.py#64
>>
>>
>> On Wed, 3 Mar 2010, Ethan Quach wrote:
>>
>>> John,
>>>
>>> This fix seems to fix the typical cases of manifest names, but there are
>>> out-lier cases that would still seem
>>> to be problematic. With this fix, manifests with name fields
>>> "foo" and "bar" will now show up as "foo" and "bar" in the
>>> list output; but manifests with name fields "foo.xml" and
>>> "foo.xml.xml", show up as "foo" and "foo.xml" respectively.
>>> Its the out-lier cases, but that seems still broken.
>>>
>>> As per your note in Comment 1,
>>>
>>># everywhere we expect manifest names to be file names so ensure
>>># the name matches
>>>if not name.endswith('.xml'):
>>>name += ".xml"
>>>return name
>>>
>>> Removing it would require additional changes within publish-manifest and
>>> probably the delete-manifest.
>>>
>>>
>>> is a subsequent bug being filed to fix that? Because really,
>>> that seems to be the underlying culprit of this
>>> inconsistency. We need to be consistent and just append
>>> '.xml' to all name fields as-is (whether or not they already
>>> end in .xml), or not append for all.
>>>
>>>
>>> thanks,
>>> -ethan
>>>
>>>
>>>
>>> On 02/26/10 16:53, John Fischer wrote:
All (especially Clay and Ethan),
Here is another webrev that I need reviewed:
http://cr.opensolaris.org/~johnfisc/12724-manifest-names
This is for:
http://defect.opensolaris.org/bz/show_bug.cgi?id=12724
The AI Database code stores manifest names as filenames (with the
extension .xml). These change adds an optional parameter
(return_fnames) to the getManNames function in the AI_database.py
with the default value of True. Thus the code generates the original
returned filenames (manifest-name.xml). The installadm list
subcommand code sets this optional parameter to False and gets the
.xml extension stripped off (the manifest name instead of the
filename).
I believe this approach is better then changing the internal database
storage of the manifests as the rest of the webserver code assumes the
manifest filename and not the manifest name.
Thanks,
John
___
caiman-discuss mailing list
caiman-discuss at opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>> ___
>>> caiman-discuss mailing list
>>> caiman-discuss at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>>
>
[caiman-discuss] Code Review for create-client pythonization
Hi Keith, I've made the correction you suggest as certainly it make much more sense for the file objects to implement these functions but I've also fixed bug 15175 "installadm delete-service can errantly error about boot archives". So, I think you have roughly 6 lines of code to please check and I'll finally get this off our plates. Differential webrev: http://cr.opensolaris.org/~clayb/11510/webrev4.diff/ Full webrev: http://cr.opensolaris.org/~clayb/11510/webrev4/ Thank you, Clay On Wed, 3 Mar 2010, Keith Mitchell wrote: > Hi Clay, > > Short version - the changes look good! (A few compliments and thank-you's > below) > > One last question though: Seems like "is_readable" and "is_writeable" belong > on the File_ and StringIO_ classes? (With FileMethods possibly implementing > intelligent "defaults", i.e. writable returning False and readable returning > True) > > (As a stylistic note, since they're properties and not plain methods, I would > have called them "readable" and "writable") > > Thanks, > Keith > > On 03/ 2/10 03:29 PM, Clay Baenziger wrote: >> Hi Keith, >> Thank you for this review! And your ability to remember and catch small >> details is not annoying -- it's a God-send for someone like me. My comments >> in-line. I've fixed your comments and added a fix for a bug I found in >> installadm_common.py when running delete-service as well: >> Differential webrev: >> http://cr.opensolaris.org/~clayb/11510/webrev3.diff >> Full Webrev: >> http://cr.opensolaris.org/~clayb/11510/webrev3 >> >> Thank you, >> Clay >> >> On Tue, 2 Mar 2010, Keith Mitchell wrote: >> >>> Hi Clay, >>> >>> I have a few comments about these fixes. >>> >>> First, a meta-question about 14735 - is "SystemExit" truly the appropriate >>> exception to be raising here, given that there's no intent to exit the >>> Python interpreter? I believe SystemExit should only be raised with the >>> intention of exiting the program, and that it should never be caught >>> (unless re-raised). There must be a more appropriate exception to raise - >>> if not, it's trivial to define one as needed. >>> http://docs.python.org/library/exceptions.html#exceptions.SystemExit >> >> I certainly feel SystemExit is more appropriate than SystemError. However, >> the big case here of run_cmd() returning SystemExit should issue its own >> exception. I have added an explicit call out for run_cmd() in 4016 - A/I >> Python code error architecture should be looked at for better reuse (i.e. >> in a different U/I). I would like to avoid massive code changes in this bug >> due to schedule constraints right now. > > Certainly SystemExit is better than SystemError! I definitely wasn't implying > a reversal of the fix :) Adding the cases in question to bug 4016 seems like > an appropriate action, given the time constraints. I do wonder at what point > we can get away from SystemExit though. Perhaps I'll put my foot down a bit > more after this release... > >> >>> create_client.py: >>> function: parse_options(): optparse.OptionParser defines a function, >>> error(), which prints out a message and usage appropriately (as well as >>> exiting). I think utilizing that functionality may be desired here, >>> instead of manually raising SystemExits. >> >> I have changed to parser.error() thank you for pointing this out. >> >>> 99-100, 145: Please remove the commented out lines >> >> Thank you >> >>> 121: My count may be off, but if you follow PEP8 and remove the spaces >>> around the '=' inside parentheses (keyword arguments shouldn't have the >>> space around the '=' as I recall), then this fits on one line cleanly. >> >> Thank you, I've fixed the parenthesized " = "'s to "=" throughout the file. >> >>> 150: Not related to your change, but this should be "...arch == 'SPARC':" >>> instead of 'is' - technically, two equivalent strings don't have to be the >>> same object. >>> (Ditto for line 274) >> >> Yes, relying on the class's hash function is a better way to go in the face >> of later change. >> >>> 160: Nit/PEP8 - No spaces around '=' in keyword args. >> >> Thank you for the reminder >> >>> 295-311: >>> Seems like some of this hoop jumping could be avoided by having separate >>> except clauses for SystemExit and com.AIImage.AIImageError. (The remainder >>> of the hoop jumping would not be needed if we didn't abuse SystemExit >>> instantiation by trying to pass in both a message and an error code in >>> other parts of our code, and instead created a simple sub-class of >>> SystemExit that had room for both if we really need it). >> >> Yes this hoop jumping was to deal with OptionParser exiting. If I can let >> it handle all exit cases, it makes life easier. > > It looks much better now! :) > >> >>> 319: Why was this moved after the i
[caiman-discuss] Code Review for create-client pythonization
Hi Keith, My apologies for the wonky merge. I think the webrevs should be fixed up now; same URLs: Differential webrev: http://cr.opensolaris.org/~clayb/11510/webrev4.diff/ Full webrev: http://cr.opensolaris.org/~clayb/11510/webrev4/ Thank you, Clay On Mon, 15 Mar 2010, Keith Mitchell wrote: > The following comments are based on the differential webrev, although it > appears that the differential webrev has a few oddities related to merging > (e.g., the find_TFTP_root changes show up in the diff). > > delete_service.py lines 207, 539, 565, 664, 711: > Were these reverted intentionally? > 525 & 528: Print statements need cleaning > > 778: Not sure what's going on with this comment. > > - Keith > > On 03/12/10 06:43 PM, Clay.Baenziger at Oracle.COM wrote: >> Hi Keith, >> I've made the correction you suggest as certainly it make much more >> sense for the file objects to implement these functions but I've also fixed >> bug 15175 "installadm delete-service can errantly error about boot >> archives". So, I think you have roughly 6 lines of code to please check and >> I'll finally get this off our plates. >> >> Differential webrev: >> http://cr.opensolaris.org/~clayb/11510/webrev4.diff/ >> >> Full webrev: >> http://cr.opensolaris.org/~clayb/11510/webrev4/ >> Thank you, >> Clay >> >> >> On Wed, 3 Mar 2010, Keith Mitchell wrote: >> >>> Hi Clay, >>> >>> Short version - the changes look good! (A few compliments and thank-you's >>> below) >>> >>> One last question though: Seems like "is_readable" and "is_writeable" >>> belong on the File_ and StringIO_ classes? (With FileMethods possibly >>> implementing intelligent "defaults", i.e. writable returning False and >>> readable returning True) >>> >>> (As a stylistic note, since they're properties and not plain methods, I >>> would have called them "readable" and "writable") >>> >>> Thanks, >>> Keith >>> >>> On 03/ 2/10 03:29 PM, Clay Baenziger wrote: Hi Keith, Thank you for this review! And your ability to remember and catch small details is not annoying -- it's a God-send for someone like me. My comments in-line. I've fixed your comments and added a fix for a bug I found in installadm_common.py when running delete-service as well: Differential webrev: http://cr.opensolaris.org/~clayb/11510/webrev3.diff Full Webrev: http://cr.opensolaris.org/~clayb/11510/webrev3 Thank you, Clay On Tue, 2 Mar 2010, Keith Mitchell wrote: > Hi Clay, > > I have a few comments about these fixes. > > First, a meta-question about 14735 - is "SystemExit" truly the > appropriate exception to be raising here, given that there's no intent > to exit the Python interpreter? I believe SystemExit should only be > raised with the intention of exiting the program, and that it should > never be caught (unless re-raised). There must be a more appropriate > exception to raise - if not, it's trivial to define one as needed. > http://docs.python.org/library/exceptions.html#exceptions.SystemExit I certainly feel SystemExit is more appropriate than SystemError. However, the big case here of run_cmd() returning SystemExit should issue its own exception. I have added an explicit call out for run_cmd() in 4016 - A/I Python code error architecture should be looked at for better reuse (i.e. in a different U/I). I would like to avoid massive code changes in this bug due to schedule constraints right now. >>> >>> Certainly SystemExit is better than SystemError! I definitely wasn't >>> implying a reversal of the fix :) Adding the cases in question to bug 4016 >>> seems like an appropriate action, given the time constraints. I do wonder >>> at what point we can get away from SystemExit though. Perhaps I'll put my >>> foot down a bit more after this release... >>> > create_client.py: > function: parse_options(): optparse.OptionParser defines a function, > error(), which prints out a message and usage appropriately (as well as > exiting). I think utilizing that functionality may be desired here, > instead of manually raising SystemExits. I have changed to parser.error() thank you for pointing this out. > 99-100, 145: Please remove the commented out lines Thank you > 121: My count may be off, but if you follow PEP8 and remove the spaces > around the '=' inside parentheses (keyword arguments shouldn't have the > space around the '=' as I recall), then this fits on one line cleanly. Thank you, I've fixed the parenthesized " = "'s to "=" throughout the file. > 150: Not relate
[caiman-discuss] Trivial Code Review: 15262 installadm lacks dependency on gawk
Hello, I've tested and changed the dependency on GNU awk to the standard /usr/bin/awk in Solaris. I only found /usr/gnu/bin/awk referenced in usr/src/cmd/installadm/svc-install-server for the entirity of the gate and /usr/gnu/bin/sed was also otherwise absent. Testing: Make the change on a system's /lib/svc/method/svc-install-server then: svcadm refresh svc:/system/install/server:default svcadm restart svc:/system/install/server:default svcadm disable svc:/system/install/server:default svcadm enable svc:/system/install/server:default Webrev: http://cr.opensolaris.org/~clayb/15262/ Bug: http://defect.opensolaris.org/bz/show_bug.cgi?id=15262 Thank you, Clay
[caiman-discuss] Updated AI Image Management design
Hi Dave, I was wondering how the envisioned changes to pxegurb would affect the menu.lst search order. My understanding is today that pxegrub searches for menu.lst. then menu.lst. and finally menu.lst in the TFTP server's root. After the change to traverse up a bootfile path, will pxegrub still search menu.lst. in all directories, then menu.lst., etc. or will it search for all files in the directory and then traverse up and search again? (Breadth or depth first?) Thank you, Clay On Fri, 12 Feb 2010, Dave Miner wrote: > As you may recall, we reviewed a first draft of this design in December/early > January. I've finally published a revision which addresses the comments I > received, primarily from Jan and Ethan. > > While not exactly frozen, implementation work is underway based on this > design so any remaining issues should be raised soon to allow the best chance > to address them. > > http://hub.opensolaris.org/bin/view/Project+caiman/AI+Image+Management > > Dave > ___ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
