Hi Sarah,
Responses inline. I removed topics I had no further comments on.
Sarah Jelinek wrote:
> Hi Keith,
>
>
> A few comments inline...
>
>> Hi Sarah,
>>
>> Thank you for the review. I'll answer your comments on the profile
>> code here (including the comments from your other response).
>>
>> - Keith
>>
>>>> 429 logging.debug("Create tgt.Disk - geometry.blocksz = " +
>>>> str(g.blocksz))
>>>> 430 logging.debug("Create tgt.Disk - geometry.cylsz = " +
>>>> str(g.cylsz))
>>>> 431 logging.debug("Create tgt.Disk - name = " + name)
>>>> 432 logging.debug("Create tgt.Disk - blocks = " +
>>>> str(blocks))
>>>> 433 logging.debug("Create tgt.Disk - controller = " +
>>>> str(controller))
>>>> 434 logging.debug("Create tgt.Disk - boot = " + str(boot))
>>>> 435 logging.debug("Create tgt.Disk - removable = " +
>>>> str(removable))
>>>> 436 logging.debug("Create tgt.Disk - vendor = " +
>>>> str(vendor))
>>>> 437 logging.debug("Create tgt.Disk - serialno = " +
>>>> str(serialno))
>>>> 438 logging.debug("Create tgt.Disk - use_whole = " +
>>>> str(use_whole))
>>>> 439 logging.debug("Create tgt.Disk - fdisk = " + str(fdisk))
>>>> 440 logging.debug("Create tgt.Disk - vtoc = " + str(vtoc))
>>>> 441 logging.debug("Create tgt.Disk - child_list_size = " +
>>>> 442 str(len(child_list)))
>>>
>>> The debug logging would be better put in a separate utility python
>>> module which you call.
>>
>> I'll check with Karen, but I think these logging statements are
>> mostly temporary.
>
> I think that having debug logging, or debug code in from the start is
> fine, I just think it should be based on something the user sets to
> trigger the debug action.
The python logging module won't add those statements to the log, unless
the user has started the installer with a "-v debug" flag. By default,
we plan on only having logging.info, warning, error and exception level
messages in the log (with 'info' roughly tracking the user's selections
through the UI).
We do plan on refining our use of the logging module - cleaning up
excessive logging, formatting the strings to improve performance,
standardizing the logging levels for consistency (i.e., making sure the
verbosity matches the intent of each logging statement).
>
>
>>>
>>>> 788 '''Returns True if it is possible to edit this
>>>> partition's size'''
>>>> 789 if self.is_extended():
>>>> 790 for logical in disk.get_logicals():
>>>> 791 if logical.id != PartitionInfo.UNUSED:
>>>> 792 return False
>>>
>>> If a logical partition is not unused, isn't it still possible for
>>> users to edit the size, which of course would potentially trash
>>> other data. Not sure why it has to be unused to be modifiable.
>>
>> The UI spec calls for the Extended partition's size to be editable if
>> and only if there are no logical partitions contained within it.
>
> Why is this? So, if I was a user and I wanted to change the extended
> partition size as part of my install, what would I do?
To keep the current logical partitions and change the size, a third
party utility such as gparted would have to be used. From within the
installer, the user can manually delete the existing logical partitions,
then resize the extended partition, or delete the extended partition and
create a new extended partition in a different 'slot.' I'll defer to
Frank for more details on the reasoning here, if more explanation is
still needed.
>>>
>>>> 47 self.is_role = is_role
>>>
>>> With the liveCD GUI, you can only enter a user which is a role. No
>>> root user is allowed now. Is this the case with the text installer?
>>> So, shouldn't is_role always be True?
>>
>> The Text Installer maintains the GUI's original behavior; that is,
>> root is created as a normal user if no primary user is defined, and
>> created as a role otherwise.
>
> Why are we doing this? We modified the GUI to only allow a user, no
> root account, so why is the text installer allowing this?
Different target audience; we conversed with Frank on this as well and
came to the conclusion that, as a 'server' install, the use case for
creating a root account is more likely, and more desired, so preserving
the original behavior seemed like the correct choice.
Thanks,
Keith
>
> thanks,
> sarah
