Skip to content

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Nov 12, 2013

No description provided.

@bdunne
Copy link
Member Author

bdunne commented Nov 12, 2013

@Fryguy @jrafanie Please review

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9acb0cb on brandondunne:move_certificate_installation_to_linux_admin into ecf649d on ManageIQ:master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call class methods with . over ::

@Fryguy
Copy link
Member

Fryguy commented Nov 12, 2013

The code looks duplicated across the two types. Can the method be moved down into a base class?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 772d77d on brandondunne:move_certificate_installation_to_linux_admin into ecf649d on ManageIQ:master.

@bdunne
Copy link
Member Author

bdunne commented Nov 12, 2013

@Fryguy Updated based on comments.

The problem is that the method doesn't make sense in the base class because the base class is used when the system is not registered.

@Fryguy
Copy link
Member

Fryguy commented Nov 12, 2013

@brandondunne That's a weird layout for the class inheritance, then. There should probably be an Unregistered class or something that lives alongside Rhn and SubscriptionManager.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling aed1712 on brandondunne:move_certificate_installation_to_linux_admin into ecf649d on ManageIQ:master.

@bdunne
Copy link
Member Author

bdunne commented Nov 12, 2013

@Fryguy updated based on conversation

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling d959867 on brandondunne:move_certificate_installation_to_linux_admin into ecf649d on ManageIQ:master.

Fryguy added a commit that referenced this pull request Nov 13, 2013
…n_to_linux_admin

Move certificate installation to linux admin
@Fryguy Fryguy merged commit c98624f into ManageIQ:master Nov 13, 2013
@bdunne bdunne deleted the move_certificate_installation_to_linux_admin branch November 13, 2013 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants