Skip to content

Conversation

@kirtiv1
Copy link
Collaborator

@kirtiv1 kirtiv1 commented Mar 14, 2019

No description provided.

@kirtiv1 kirtiv1 requested a review from dimaspivak March 14, 2019 18:20

class Kerberos_Helper:

def __init__(self, network, operating_system):
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a docstring to the class reflecting this init? Also, since it looks like operating_system is a version and not the whole OS name, can we update the naming to make that clear?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@dimaspivak dimaspivak left a comment

Choose a reason for hiding this comment

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

Just tiny suggestions. Can we update the README to give an example of using the new args?

start.py Outdated
nodes = [primary_node] + secondary_nodes
if args.kerberos:
logger.info('Creating KDC node...')
kdc = kerberos_helper.Kerberos_Helper(args.network, args.operating_system)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to call the class Kdc in the first place if it'll be instantiated and treated like one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the instance variable name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just tiny suggestions. Can we update the README to give an example of using the new args?

Done.

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.

2 participants