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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code questions #10

Closed
Manbearpiet opened this issue Sep 9, 2023 · 2 comments
Closed

Code questions #10

Manbearpiet opened this issue Sep 9, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@Manbearpiet
Copy link

Manbearpiet commented Sep 9, 2023

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 馃槃馃憤.

@azurekid azurekid self-assigned this Sep 12, 2023
@azurekid azurekid added enhancement New feature or request good first issue Good for newcomers labels Sep 12, 2023
@azurekid
Copy link
Member

Thank you @Manbearpiet for your detailed review and code improvement suggestions.
Much appreciated that you have taken the time to go through the code.

Get-AccessToken and Get-LogAnalyticsWorkspace

In some cases I have created my own functions like Get-AccessToken and Get-LogAnalyticsWorkspace. Main reason to this is to limit the dependency on other modules, especially when using in automation scenario's I am a fan of a minimal footprint like in GitHub, DevOps or using in Function Apps.

The Invoke-AzRestMethod is not always very stable and consistent in the output of the queried resources, so trying to avoid that a little bit also.

FunctionNames

I was hoping Microsoft would pick this up in their own repository as an extension to the already existing Microsoft modules. 馃槉

Others

I will pick-up the other suggestions, and appreciate if I could add you as a reviewer on the PR.
Pester Testing is something I do need to add, but didn't had the inspiration yet to think of what I would like to test.

@Manbearpiet
Copy link
Author

Sure mate, happy to help!

If you need input apart from PRs you know where to find me 馃槃.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
Development

When branches are created from issues, their pull requests are automatically linked.

2 participants