[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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





[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]