Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for kerberization #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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