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

Use more natural property (ScopeId) as a key for the xDhcpScope resource. #48

Closed
bielawb opened this issue Jun 22, 2018 · 3 comments · Fixed by #49
Closed

Use more natural property (ScopeId) as a key for the xDhcpScope resource. #48

bielawb opened this issue Jun 22, 2018 · 3 comments · Fixed by #49

Comments

@bielawb
Copy link
Contributor

@bielawb bielawb commented Jun 22, 2018

I tried to use xDhcpScope in production and while testing different scenarios I noticed that key properties for that resource are IPStartRange and IPEndRange.

In my opinion it would be far better to use ScopeId as a key property - it should also help simplifying the code and making most of the properties of the scope write-able. Get-DhcpServerv4Scope allows user to specify ScopeId to limit the results, so with that property as a key validating presence/properties should be easier and more efficient.

I do realize that it would be a serious breaking change, but I believe it's a necessary one.

@johlju
Copy link
Member

@johlju johlju commented Jun 22, 2018

If ScopeId is always unique, and two scope id's cannot be added using the same IP range, then I sounds possible to change this. Although, maybe it should be possible to add two scope id's with the same IP range?
With the current implementation the configuration will fail on compilation if the same IP range is used, than during runtime if scope id is used as key?

Is the general consensus is that this change would be better than current implementation?

@bielawb
Copy link
Contributor Author

@bielawb bielawb commented Jun 24, 2018

IP range is a property of scope with given ScopeId - there is no problem to change the size of existing range with Set-DhcpServerV4Scope. ScopeId is enough to reliably identify the scope, start/end expose that information too (implicitly - StartRange -band SubnetMask -> ScopeId), but filtering Get-DhcpServerV4Scope results with these properties means no way to adjust size of existing scope. Switching to ScopeId as Key property makes it more explicit, removes a need of filtering results from Get-DhcpServerV4Scope and last but certainly not least for me: makes it possible to resize existing scopes. I created PR to change it, but I'm open to any feedback why that would make no sense.

I also found few small bugs here and there and collapsed few tests using TestCases instead of X almost identical tests.

@johlju
Copy link
Member

@johlju johlju commented Jun 25, 2018

@bielawb I agree that it makes more sense using scope id as key. This will be a breaking change as you mentioned, so I label it as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants