[Pulp-list] Agent: Package.install() API

Jeff Ortel jortel at redhat.com
Thu Nov 10 14:03:59 UTC 2011


Thanks for the comments, Jay.

On 11/09/2011 02:54 PM, Jay Dobies wrote:
> Just to make sure I'm reading this right, these calls are made by the Pulp server against
> consumers right?
>
>> 1) change:
>> Packages.install(names, reboot, assumeyes)
>> to:
>> Packages.install(names, reboot, importkeys)
>>
>> The affect of "assumeyes" here is /really/ to permit YumBase to import
>> GPG keys as needed. Although, it's implemented in YumBase as
>> "assumeyes", from an API perspective, "importkeys" seems clearer.
>
> +1
>
>> 2) change the return value of Packages.install()
>> from:
>> [[installed], [reboot_requested, rebooted]]
>> to:
>> { installed : [], rebooted : <bool> }
>>
>> A dict is clearer than coded list/tuples. Also, the 'reboot_requested'
>> flag is really not needed. The caller know that a reboot after package
>> install was requested. The 'rebooted' flag indicates whether the
>> 'reboot' request actually performed. The consumer.conf can disallow
>> reboots.
>
> I like the change, just curious how it works. If I'm reading this right, the server calls
> Packages.install() on the consumer. What you're proposing here is the return value. How's
> that work from a technical standpoint if the consumer reboots in the middle of that call?

Perhaps 'rebooted' being past tense suggests that the machine has already been rebooted 
but what it means is the the 'shutdown -r <delay>' command has been executed.  The delay 
is designed to allow time for the RMI reply to occur.  Maybe the return should be:

{ installed : [], reboot_scheduled : <bool> }

>
>> 3) clarify the consumer.conf:
>>
>> replace:
>>
>> [client]
>> reboot_schedule = 3
>> import_gpg_keys = False
>> #assumeyes = False
>>
>> with:
>>
>> [reboot]
>> permit = False
>> delay = 3
>>
>> [gpg]
>> permit_import = False
>>
>> Comments?
>
> Am I the only one who read "assumeyes" as "assume eyes"? I like the change.
>
> The permit flags are a really good idea, especially for reboot. I like the idea of being
> able to configure the consumer to not be rebooted when Pulp tells it to. It looks like
> you're suggesting reboots to be disabled by default, which I think is probably the right
> call.
>
> Can you change the delay key to be something like "delay_in_min"? I'm a big fan of keys
> having their units built right into the name; makes it easier on the user in case we
> forget .conf file comments and easier on the developer so they know what it is they are
> pulling in when they retrieve that value.

I kind of lean toward putting the units in the comments along with all the other 
information about the property (I know, big surprise).  But, since you took the time to 
review this and comment ... how about I make it:

delay_minutes

>
>




More information about the Pulp-list mailing list