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

VMNetworkAdapter: add parameter IgnoreNetworkSetting #189

Closed
wants to merge 2 commits into from

Conversation

stehlih
Copy link
Contributor

@stehlih stehlih commented Jun 17, 2021

Pull Request (PR) description

xVMNetworkAdapter: add parameter IgnoreNetworkSetting

Set the new parameter to True if the guest operating system configures the network adapter.
In this case all ip settings including DHCP are skipped.
The paramter is set to False as default to keep the current behavior.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@johlju
Copy link
Member

johlju commented Jun 8, 2022

We have renamed the resource, removing 'x', so please rebase this PR.

@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jun 8, 2022
@johlju johlju changed the title xVMNetworkAdapter: add parameter IgnoreNetworkSetting VMNetworkAdapter: add parameter IgnoreNetworkSetting Jun 8, 2022
@johlju johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 8, 2022
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @johlju and @stehlih)

a discussion (no related file):
Looking at this change i thinking that adding a property for IgnoreNetworkSetting is not the right move. Would it not be better to refactor the resource to not enforce the existing property NetworkSetting if is is not specified in the configuration. if $PSBoundParameters.ContainsKey('NetworkSetting') then the resource should enforce (test and set) the property.


@johlju johlju self-requested a review June 9, 2022 10:57
@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jun 9, 2022
@johlju
Copy link
Member

johlju commented Jun 9, 2022

@stehlih let me know if my suggestion in the review comment does not work?

@stehlih
Copy link
Contributor Author

stehlih commented Jun 10, 2022

Your proposal is the best way but it changes the current behavior of the resource and DHCP will not enabled explicitly if the NetworkSettings parameter is not specified (-> this was an unexpected behavior for me because it had have some side effects on my guest VM).
I can make the code changes in the next 2 weeks and update my pull request.

@johlju
Copy link
Member

johlju commented Jun 11, 2022

Happy to see the changes.

Should we also add a property Dhcp in the schema class NetworkSettings so we can easily determine if DHCP or static IP should be used? That I think would cover the scenarios:

  • If NetworkSettings parameter is not part of configuration, do not enforce IP (DHCP or Static).
  • If NetworkSettings parameter is passed
    • and contain DHCP -eq $true then enable DHCP.
    • and contain DHCP -eq $false then use Static IP.

Since we have renamed the resources and are releasing the module as a new major version we can also add this as a breaking change.

I will wait for the updated PR. Many thanks and great work @stehlih! 🙇‍♂️

@johlju
Copy link
Member

johlju commented Jun 12, 2022

When this PR is reworked according to previous comment I think it will also close the issue #191, but would be great to add a test with the set of parameter as seen in the issue.

@johlju
Copy link
Member

johlju commented Jun 12, 2022

FYI when rebasing. The resource and parameter descriptions has been removed from the README.md and moved to the schema.mof - the parameter descriptions are automatically generated for the WIki dokcumentation using the schema.mof and the resource's README.md.

@stehlih stehlih closed this Jun 23, 2022
@stehlih stehlih deleted the master branch June 23, 2022 19:09
@johlju johlju added closed by author The issue or pull request was closed by the author. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed by author The issue or pull request was closed by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants