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

Refactor win_domain_member.ps1 and change comments #52391

Open
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
6 participants
@ChrisRo89
Copy link

ChrisRo89 commented Feb 16, 2019

SUMMARY

Did some refecatorings of the PowerShell code.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

win_domain_member.ps1

ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

@bcoca bcoca changed the title Refactored module and change comments Refactored win_domain_member and changed comments Feb 19, 2019

@samdoran samdoran changed the title Refactored win_domain_member and changed comments Refactor win_domain_member.ps1 and change comments Feb 19, 2019

@samdoran samdoran removed the needs_triage label Feb 19, 2019

.Parameter passwordEncrypted
Encrypted password of the user which will be used for join or unjoin the computer object
#>
function Save-Credential {

This comment has been minimized.

@jborean93

jborean93 Feb 25, 2019

Contributor

We aren't actually "Saving" a credential, the docs for Save is

Preserves data to avoid loss.

https://docs.microsoft.com/en-us/powershell/developer/cmdlet/approved-verbs-for-windows-powershell-commands

Technically New or ConvertTo-Credentials would be a better fit here. I also don't see the main point of now accepting only a SecureString. It seems like the main benefit before was that it was a helper function to easily convert the string inputs from Ansible to a PSCredential whereas now we still need to do some work outside to convert the pass to a SecureString.

In reality I don't see the need for this cmdlet as it's only being called once. If you are going to refactor this part I would remove it altogether and just create the credentials where this is being called.

Function Get-HostnameMatch {
Param(
[string] $hostname
)

# Add-Computer will validate the "shape" of the hostname- we just care if it matches...

This comment has been minimized.

@jborean93

jborean93 Feb 25, 2019

Contributor

What's your reasoning for removing this?

@@ -100,17 +108,13 @@ Function Join-Domain {
Param(
[string] $dns_domain_name,
[string] $new_hostname,
[string] $domain_admin_user,
[string] $domain_admin_password,
[pscredential] $domainCredential,

This comment has been minimized.

@jborean93

jborean93 Feb 25, 2019

Contributor

Common PS syntax is to just use credential, I don't see a reason to specify a specific type of credential now that trying to move across to a more ps-like syntax.

@@ -161,16 +165,14 @@ Function Set-Workgroup {
Function Join-Workgroup {
Param(
[string] $workgroup_name,
[string] $domain_admin_user,
[string] $domain_admin_password
[pscredential] $domainCredential

This comment has been minimized.

@jborean93

jborean93 Feb 25, 2019

Contributor

Same as above, just have this as credential.

@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Feb 28, 2019

@ansibot ansibot removed the stale_ci label Feb 28, 2019

Christian.Rohr added some commits Mar 3, 2019

@ansibot ansibot removed the ci_verified label Mar 3, 2019

Christian.Rohr Christian.Rohr
@acozine

This comment has been minimized.

Copy link
Contributor

acozine commented Mar 21, 2019

Not sure why the bot marked this as a docs PR - I don't see a docs component. Removing the label, but feel free to ping me if you want my input.

@acozine acozine removed the docs label Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.