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

Discussion: Deprecate WeakHostSend, WeakHostRecieve and DHCPClient #360

Closed
PlagueHO opened this issue Oct 12, 2018 · 6 comments · Fixed by #368
Closed

Discussion: Deprecate WeakHostSend, WeakHostRecieve and DHCPClient #360

PlagueHO opened this issue Oct 12, 2018 · 6 comments · Fixed by #368
Assignees
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. in progress The issue is being actively worked on by someone.

Comments

@PlagueHO
Copy link
Member

The WeakHostSend, WeakHostRecieve and DHCPClient resources all configure properties on the *-NetIPInterface. They are "single purpose" resources that result in a lot of duplicated code/function. E.g. the resources are practically identical (duplicated code, tests etc).

I'm currently implementing a more generic NetIPInterface resource that can be used to configure properties on the *-NetIPInterface as part of #355. I'm excluding the WeakHostSend, WeakHostRecieve and DHCPClient properties as they are covered by these other resources.

But it seems to me to be non-trivial duplication of function/tests. This would reduce the amount of code and tests needing to be maintained and also simplify configurations - but of course would be a BREAKING CHANGE.

For example, the following current config:

Configuration Example
{
    Import-DscResource -Module NetworkingDsc

    Node localhost
    {
        DhcpClient EnableDhcpClient
        {
            InterfaceAlias = 'Ethernet'
            AddressFamily  = 'IPv4'
            State          = 'Enabled'
        }

        WeakHostSend DisableWeakHostReceiving
        {
            InterfaceAlias = 'Ethernet'
            AddressFamily  = 'IPv4'
            State          = 'Disabled'
        }

        WeakHostSend DisableWeakHostSending
        {
            InterfaceAlias = 'Ethernet'
            AddressFamily  = 'IPv4'
            State          = 'Disabled'
        }
    }
}

Would be changed to:

Configuration Example
{
    Import-DscResource -Module NetworkingDsc

    Node localhost
    {
        NetIPInterface ConfigureNetIPInterface
        {
            InterfaceAlias = 'Ethernet'
            AddressFamily  = 'IPv4'
            DHCP          = 'Enabled'
            WeakHostSend = 'Disabled'
            WeakHostRecieve = 'Disabled'
        }
    }
}

@tysonjhayes , @johlju - what are your thoughts on this?

@PlagueHO PlagueHO added discussion The issue is a discussion. breaking change When used on an issue, the issue has been determined to be a breaking change. labels Oct 12, 2018
@johlju
Copy link
Member

johlju commented Oct 17, 2018

I suggest deprecate the resource, merging them into the new resource - and in a later PR (maybe when doing another breaking change) remove them.
We had a similar scenario in SqlServerDsc a while back - I waited a couple of releases, then I removed them as a breaking change.

@PlagueHO
Copy link
Member Author

PlagueHO commented Oct 18, 2018

Cool. That what I was thinking. I'll leave this issue open for a while before doing anything on it to give community time to provide feedback.

@stale
Copy link

stale bot commented Nov 17, 2018

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.

@stale stale bot added the stale The issue or pull request was marked as stale because there hasn't been activity from the community. label Nov 17, 2018
@PlagueHO
Copy link
Member Author

Bumping to keep this open for now.

@stale stale bot removed the stale The issue or pull request was marked as stale because there hasn't been activity from the community. label Nov 17, 2018
@stale
Copy link

stale bot commented Dec 17, 2018

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.

@stale stale bot added the stale The issue or pull request was marked as stale because there hasn't been activity from the community. label Dec 17, 2018
@PlagueHO PlagueHO added enhancement The issue is an enhancement request. and removed stale The issue or pull request was marked as stale because there hasn't been activity from the community. labels Dec 17, 2018
@PlagueHO
Copy link
Member Author

I will plan to implement this change over the next few weeks.

@PlagueHO PlagueHO added in progress The issue is being actively worked on by someone. and removed discussion The issue is a discussion. labels Dec 22, 2018
@PlagueHO PlagueHO self-assigned this Dec 22, 2018
PlagueHO added a commit that referenced this issue Jan 22, 2019
BREAKING CHANGE: Added Dhcp, WeakHostReceive and WeakHostSend parameters to NetIPInterface and deprecates MSFT_DHCPClient, MSFT_WeakHostReceive, MSFT_WeakHostSend - Fixes #360
@SteveL-MSFT SteveL-MSFT added this to In progress in powershell/dscresources May 14, 2019
@SteveL-MSFT SteveL-MSFT removed this from In progress in powershell/dscresources Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. in progress The issue is being actively worked on by someone.
Projects
None yet
2 participants