sgtoj / PSConnectWise

PowerShell Module that provide several CmdLets to interact with ConnectWise REST API service.
MIT License
45 stars 13 forks source link

Updated Cmdlets to Automatically Add Server Connection Information #6

Closed Persistent13 closed 8 years ago

Persistent13 commented 8 years ago

Hello @sgtoj

I've re-written the cmdlets to automatically add the server connection information once entered into the Get-CWConnectionInfo cmdlet.

I've done so by placing the Domain, CompanyName, PublicKey, and PrivateKey variables into scoped variables that are available to the module but not the end user.

I've also added the pararmters to each cmdlet in the module so if required or preferred by the end user they may still run the cmdlets without first running the Get-CWConnectionInfo cmdlet.

Some examples of the change below:

Get-CWConnectionInfo -Domain 'support.acme.com' -CompanyName 'acme' -PublicKey 'qwerty' -PrivateKey '1234567'

Domain      : support.acme.com
CompanyName : acme
PublicKey   : qwerty
PrivateKey  : 1234567
CodeBase    : v4_6_release/
BaseUrl     : https://support.acme.com/v4_6_release/apis/3.0
Header      : {x-cw-overridessl, Authorization, Accept, Type}
ApiVersion  : 3.0
OverrideSSL : True

Get-CWServiceBoard -ID 1

id                 : 1
name               : Service Desk Tier 1
locationId         : 11
businessUnitId     : 10
inactive           : False
signOffTemplateId  : 1
sendToContact      : False
contactTemplateId  : 0
sendToResource     : True
resourceTemplateId : 0
_info              : @{lastUpdated=2016-04-20T18:21:07Z; updatedBy=john}

And without running Get-CWConnectionInfo

Get-CWServiceBoard -ID 1 -Domain 'support.acme.com' -CompanyName 'acme' -PublicKey 'qwerty' -PrivateKey '1234567'

id                 : 1
name               : Service Desk Tier 1
locationId         : 11
businessUnitId     : 10
inactive           : False
signOffTemplateId  : 1
sendToContact      : False
contactTemplateId  : 0
sendToResource     : True
resourceTemplateId : 0
_info              : @{lastUpdated=2016-04-20T18:21:07Z; updatedBy=john}

I've adjust all the pester tests and verified them working with this change.

Let me know if you have any questions!

Thanks, Dakota Clark

sgtoj commented 8 years ago

I like the idea. In fact, it is one of my original long term plans. I didn't implement it from the beginning because I wanted to review other 3rd-party POSH modules first. So that I could research how the other modules are implementing this type of pattern and what common practices that are followed.

Why wait? We can add it now. That said, I believe it should be implement slightly different. The end result would be the same. Instead of the functions asking for the domain, company, and keys, ask only for the CWApiRestConnectionInfo object that's saved as a script variable.

Proposal

  1. Get-CWConnectionInfo saves the object to a single global/script scoped variable.
  2. Name the variable with the $CWServer, $CWServerInfo, or something prefixed with CW or ConnectWise.

Reasons

  1. By passing domain, company, and key by values, it will eventually create a CWApiRestConnectionInfo object up the stack.
  2. Each time a CWApiRestConnectionInfo object is created, it will make web request call to the CW server to find its code base (via _setCodeBase()). So it is not efficient.
  3. 3 less variables to be added to the global/script namespace.
  4. The proposed change would cause breaking changes. (I already use this module in production)
Persistent13 commented 8 years ago

No problem.

I'll be updating the code and I'll post a comment when I'm complete.

Persistent13 commented 8 years ago

Hi @sgtoj

I've updated the pull request with the changed you asked for; no issues with the pester tests either.

Let me know what you think when you get the chance.

Thanks, Dakota Clark

sgtoj commented 8 years ago

Thank you! Took me longer than I expected. Because I have made several changes in the past couple weeks, I had to fix many conflicts with the master branch. None the less, your contributions were merged with the master.