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

win_wakeonlan: New module to send Wake-On-Lan packets #26232

Merged
merged 1 commit into from
Jul 9, 2017

Conversation

dagwieers
Copy link
Contributor

SUMMARY

This is the Windows implementation of the wakeonlan module.

Useful if you want to wake up systems in a remote network with only Windows systems.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

win_wakeonlan

ANSIBLE VERSION

v2.4


# Broadcast payload to network
$UDPclient = new-Object System.Net.Sockets.UdpClient
$UDPclient.Connect($broadcast, $port)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider passing $check_mode to Wake-On-Lan function and only wrapping L55-56 in 'if (-not $check_mode) to ensure the mac format validation happens even in check mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, that's good feedback.

I mostly translated it from my python version, and I am not doing that there. So I am going to backport this there too. Plus integration test (not testing if it actually works, but at least gives some coverage).

@jhawkesworth
Copy link
Contributor

Looks good.

see inline comment regarding doing as much as possible even in check mode but otherwise this looks good to go.

@dagwieers dagwieers force-pushed the win_wakeonlan branch 3 times, most recently from 6f7dbd8 to fab7d7f Compare June 29, 2017 13:49
@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. test_pull_requests windows Windows community labels Jun 29, 2017
@ansibot
Copy link
Contributor

ansibot commented Jun 29, 2017

@SamLiu79 @timothyvandenbrande @ar7z1 @blakfeld @brianlloyd @chrishoffman @if-meaton @joshludwig @petemounce @schwartzmx @smadam813

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@jhawkesworth
Copy link
Contributor

shipit

@jhawkesworth
Copy link
Contributor

I am happy to merge this one if anyone else can review this too.

Copy link
Contributor

@ar7z1 ar7z1 left a comment

Choose a reason for hiding this comment

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

shipit

@mikeeaton83
Copy link
Contributor

looks good

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jun 29, 2017

# If we don't end up with 12 hexadecimal characters, fail
if ($mac.Length -ne 12) {
Fail-Json @{} "Incorrect MAC address length: $mac_orig"
Copy link
Contributor

Choose a reason for hiding this comment

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

Fail-Json $result ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, within the function $result was not exposed, but since we don't have a function, this is no longer an issue.

$params = Parse-Args $args -supports_check_mode $true
$check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false

$mac = Get-AnsibleParam -obj $params -name "mac" -type "str"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be mandatory like wakeonlan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, fixed.

changed = $true
}

Function Wake-On-Lan($mac, $broadcast, $port, $whatif) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the point of having this in a function, can we just make it simpler by having the code outside? Also you don't need to pass in $whatif, you can access $check_mode from within this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done exactly like the python version. But there's no real point, it's even easier not to do it :-)

version_added: '2.4'
short_description: Send a magic Wake-on-LAN (WoL) broadcast packet
description:
- The C(wakeonlan) module sends magic Wake-on-LAN (WoL) broadcast packets.
Copy link
Contributor

Choose a reason for hiding this comment

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

C(win_wakeonlan)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup !

default: 7
author:
- Dag Wieers (@dagwieers)
todo:
Copy link
Contributor

Choose a reason for hiding this comment

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

is todo a valid document entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not being exposed, which is fine. But it could be processed if needed.
Best of both worlds IMO.

@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Jun 29, 2017
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jun 30, 2017
This is the Windows implementation of the **wakeonlan** module.
Useful if you want to wake up systems in a remote network with only
Windows systems.
@dagwieers
Copy link
Contributor Author

Thanks for all the feedback ! Looking even better now :-)

@ansibot ansibot removed the shipit This PR is ready to be merged by Core label Jun 30, 2017
@jborean93 jborean93 merged commit 52c1a19 into ansible:devel Jul 9, 2017
@jborean93
Copy link
Contributor

merged, thanks @dagwieers

@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants