Skip to content
This repository has been archived by the owner on Apr 27, 2022. It is now read-only.

Move __register() and sub functions to DHCP class #75

Open
chemistry-sourabh opened this issue Feb 22, 2017 · 11 comments
Open

Move __register() and sub functions to DHCP class #75

chemistry-sourabh opened this issue Feb 22, 2017 · 11 comments

Comments

@chemistry-sourabh
Copy link
Contributor

Currently the __register method generated the .ipxe and mac address files using two other helper functions and they are part of BMI class.

Should they be moved to DHCP class or make a new TFTP class and include them ?

@ravisantoshgudimetla
Copy link
Contributor

@chemistry-sourabh please add line numbers and references in github issue. The more information you provide, it becomes much easier to discuss without going back and forth.

This idea is interesting. TBH, we didn't think about that when writing this piece. I guess first we need to set the expectation from our side straight. Dnsmasq(the one which we currently have , includes both DHCP and TFTP servers) and we need to support that for sure. For most of DHCP servers they don't include TFTP server. So, no point in making a class for that. But I think the functions that generate these files should be part of DHCP server like __generate_ipxe and others. Let me know, what you think.

@chemistry-sourabh
Copy link
Contributor Author

We can add the API calls to the DHCP class as DNSmasq supports it, to support other dhcp servers that dont have TFTP in them, we can create a tftp class for that server and use composition. Let me know if this makes sense to you.

@ravisantoshgudimetla
Copy link
Contributor

So, instead of naming it as DHCP+TFTP why can't we have something like class PXEServer. It should always include both DHCP and TFTP? - (We can have an interface for this as well).

@chemistry-sourabh
Copy link
Contributor Author

I guess this makes more sense. From now on Dnsmasq shall be called as PXEServer not DHCP (:P)

@chemistry-sourabh
Copy link
Contributor Author

@ravisantoshgudimetla help wanted ? for what ?

@ravisantoshgudimetla
Copy link
Contributor

Means, external contributors can come in and help.

@ravisantoshgudimetla
Copy link
Contributor

ravisantoshgudimetla commented Apr 14, 2017

BTW, do we have unit-tests for this class? If not, this becomes a high priority thing for release 0.5
cc @naved001.

@ravisantoshgudimetla
Copy link
Contributor

@chemistry-sourabh Do we have unit tests for this? if not, this becomes high priority.

@chemistry-sourabh
Copy link
Contributor Author

@ravisantoshgudimetla This class only has 1 method that gets IP of a node, which is very OS specific. When we move the functions to this class we can add the test cases with it.

@ravisantoshgudimetla
Copy link
Contributor

Is this part of milestone 0.5?

@chemistry-sourabh
Copy link
Contributor Author

no @ravisantoshgudimetla

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants