Code review comment for lp:~bbaude/cloud-init/rh_subscription

Revision history for this message
Scott Moser (smoser) wrote :

Brent,
 this looks good, thanks for submitting.
 a couple other minor things

a.) please don't use 'log.info' more than once.
  that goes to the console or cloud-init's stdout. and i dont really want cloud-init to spam the console with "everything looks good" types of messages.
  a single "registered with rhn" is probably fine.
  others can be debug.
  I do hope to have better rules in the future on what messages should go at what version.

b.) some unit tests would be nice.

c.) is there a reason for the hard coded /bin/subscription-manager ?
   my preference is to always assume PATH is sane. if not, something else is probably wrong.

d.) instead of _captureRun, or even subprocess. use util.subp.
 just nicer to have a single path for all that stuff.

thanks.

« Back to merge proposal