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

Add a new method add_host_with_statements #27

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

sagarun
Copy link
Contributor

@sagarun sagarun commented Sep 18, 2018

We want to set hostname, routers and domain in the dhcp lease file.
The method add_host_with_statements accepts router,domain and hostname
and sets those statements using omapi in the lease file. The method
also handles case when any of the statements are missing.

@cygnusb
Copy link
Contributor

cygnusb commented Sep 18, 2018

Thanks for your contribution. Please provide a pull request without the version bump. Thanks!

@sagarun
Copy link
Contributor Author

sagarun commented Sep 18, 2018

@cygnusb Done. Thanks.

@cygnusb
Copy link
Contributor

cygnusb commented Sep 18, 2018

Please create one clean merge request containing only the minimal changeset and not version change and revert of version change. Thanks.

@sagarun
Copy link
Contributor Author

sagarun commented Sep 18, 2018

@cygnusb I did revert the version bump. Do you see it? If you'd like to merge this as a single commit, you can 'squash merge' it. If not i can make this into a single commit. Let me know.

@sagarun
Copy link
Contributor Author

sagarun commented Sep 18, 2018

@cygnusb Just made it into a one clean commit now. Removed the version stuff.

@cygnusb
Copy link
Contributor

cygnusb commented Sep 19, 2018

Please change the function parameters to not be kwargs. For the optional parameters you can use None as default value.

@sagarun
Copy link
Contributor Author

sagarun commented Sep 19, 2018

@cygnusb Yes. I have removed the kwargs.

@cygnusb
Copy link
Contributor

cygnusb commented Sep 19, 2018

Please add documentation for all parameters. See other functions as reference

@cygnusb
Copy link
Contributor

cygnusb commented Sep 19, 2018

And please reorder parameters, to be in sync with def add_host_supersede_name.
Wouldn't it be better to name this function add_host_supersede and have multiple optional parameters be present?

@sagarun
Copy link
Contributor Author

sagarun commented Sep 19, 2018

@cygnusb reordered the parameters, renamed the method to add_host_supersede, added doctrings about the parameter types. Thanks for taking time to review. Let me know if you want me to change anything else.

@cygnusb
Copy link
Contributor

cygnusb commented Sep 19, 2018

The check for name, ip and mac is now obsolete and can be removed. In addition also the "if ip" statement below can be removed.
Afterwards I think this can be merged. Thanks!

We want to set hostname, routers and domain in the dhcp lease file.
The method add_host_supersede accepts router,domain and hostname
and sets those statements using omapi in the lease file. The method
also handles case when any of the statements are missing/not passed
by the user.
@sagarun
Copy link
Contributor Author

sagarun commented Sep 19, 2018

@cygnusb Done. Thanks.

@cygnusb cygnusb merged commit 2354b6c into CygnusNetworks:master Sep 19, 2018
@cygnusb
Copy link
Contributor

cygnusb commented Sep 19, 2018

Thank you!

@sagarun
Copy link
Contributor Author

sagarun commented Sep 19, 2018

@cygnusb Any chance it will get pushed to pypi? I still see 0.6 in pypi, can you make a release please?

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