-
Notifications
You must be signed in to change notification settings - Fork 16
Move __register() and sub functions to DHCP class #75
Comments
@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. |
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. |
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). |
I guess this makes more sense. From now on Dnsmasq shall be called as PXEServer not DHCP (:P) |
@ravisantoshgudimetla help wanted ? for what ? |
Means, external contributors can come in and help. |
BTW, do we have unit-tests for this class? If not, this becomes a high priority thing for release 0.5 |
@chemistry-sourabh Do we have unit tests for this? if not, this becomes high priority. |
@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. |
Is this part of milestone 0.5? |
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 ?
The text was updated successfully, but these errors were encountered: