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
_______________________________________________
Pulp-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/pulp-list
