securehats / azworkspacemanager Goto Github PK

When an invalid resource group name is provided an execption is shown instead of a friendly message

Describe the bug
When providing a group name that contains spaces, an error is shown that the name does not match the expected pattern

To Reproduce
Steps to reproduce the behavior:

1. Add-AzWorkspaceManagerGroup
2. Use a name with space in it
3. See error

Expected behavior
The names supported that are also allowed by the API.

Screenshots

Hey @azurekid , awesome repo dude very shiny and feature-rich good job! 😄👍
Sorry for taking so long, I was a bit tired but I found the time to look at your repo.
Thanks for the request to take a look. I'll sum up my findings and I am curious what you think of them.

Generic

try/catch

You use catch blocks with errors in them, while if someone uses $erroractionpreference = 'Continue" this'll continue around the error statement. You've entered a break statement, but these should be used around trap statements and switch statements. I personally prefer to use a throw statement in the try block and a $PSCmdlet.ThrowTerminatingError($_) in the catch block. But I'm going to check my sources on this 😄👍.

Parameter attributes

This is maybe a bit nitpicky so beware.
In several of your functions your parameters have Mandatory and ValueFromPipelineByPropertyName attributes.
Both with:

[Parameter(Mandatory = $true, ValueFromPipelineByPropertyName = $true)]

The Mandatory and ValueFromPipelineByPropertyName in there implicitly already means it is true.
This is the same effectively:

[Parameter(Mandatory, ValueFromPipelineByPropertyName)]

A parameter being not Mandatory and just accepting ValueFromPipelineByPropertyName is just leaving out the non-wanted attributes. I.e. this being a non-mandatory parameter accepting pipeline values based on property name.

[Parameter(ValueFromPipelineByPropertyName)]

Functionnames

This is a personal preference thing, while I like the Az prefixes of the cmdlets, I'd personally always choose names that makes it clear you're not creating a Microsoft-made Azure Module (recognizable with the Az* in the noun). So personally I'd prefix my functionnames with something like AzWM or something, to create a distinction with Microsoft modules and preventing potential conflicts/confusion with function names/functionality.

Format-Result

There's a return statement at the end, return statements at the end aren't necessary.
They are useful when trying to return to a higher level scope early in a function:

function Format-Result {
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true)]
        [array]$Message
    )
        $result = @()

        foreach ($value in $Message) {
        $split = $value.id.Split('/')
            $result += [ordered]@{
                Name              = $split[-1]
                ResourceGroupName = $split[-9]
                ResourceType      = '{0}/{1}' -f $split[-3], $split[-2]
                WorkspaceName     = $split[-5]
                ResourceId        = $value.id
                Tags              = $value.tags
                Properties        = $value.properties
            } | ConvertTo-Json -Depth 10 | ConvertFrom-Json -Depth 10
        }
   $result
}

Maybe the author of the code has a background in C#/.NET where this is a common practice.
This is also applicable for several other functions.

Get-AccessToken

You've created custom functionality around fetching a token, potentially to circumvent a dependency on Az.Accounts.
But you've an implicit dependency on the customer being logged in and them having the Azure Profile file present.
So wouldn't it be simpler to just do Get-AzAccesstoken -ResourceTypeName Arm? Less code for you to test and maintain 😄.

Get-LogAnalyticsWorkspace

I see some custom functions build around calling REST-methods on Azure Resource Manager API's and building the right URI and setting the auth headers, don't know if you're familiar with Invoke-AzRestMethod it does some of the heavy lifting for you.

Invoke-AzWorkspaceManager

This accepts pipeline input but contains no process block. This is advised if you're feeding more than one object in a pipeline statement. Else you'll end up with just the last object, you can read more about it in here: UseProcessBlockForPipelineCommand

Testing

Your module provides excellent functionality on Workspace Manager functionality. It can be convenient to add some code tests to test if your functions behave like you expect them to. Pester can be an excellent testing framework to test your functions in. It would help in making sure you've accounted for expected usage scenario's and validate if future features implement breaking changes in existing code. With those tests you can release future versions in confidence.

A thanks to you is also in place today I learned about $MyInvocation thanks for that 😄.

I also thought of some comfort features like tab completion on the workspaces etc., but that is not necessary.
I haven't digged around in the Workspace Manager functionality itself, I'd have to get some flight hours with it before I use your module to fully understand what it's value is and provide in depth feedback. But all in all good job dude 😄👍.

Describe the bug

When creating an assignment the payload is shown that is sent to the API

To Reproduce

1. Create and assignment Add-AzWorkspaceManagerAssignment -WorkspaceName -'myWorkspace' -GroupName 'myGroup'

Expected behavior
No payload output is shown

Screenshots
If applicable, add screenshots to help explain your problem.