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

Add support for updating Az modules with parameter AzureModuleClass #6

Merged
merged 10 commits into from
Jun 13, 2019

Conversation

ayoung012
Copy link
Contributor

If the parameter AzureModuleClass is set to 'Az', the runbook will attempt to update all modules only using Az modules itself. The script takes care not to use any conflicting modules from AzureRM.

Default behavior is the same: to use AzureRM modules throughout.

issue #5

Note: if for some reason you have installed both AzureRM and Az modules in your automation account, this script will still attempt to update all of them. It is perhaps desirable to present the user with a warning instead...

@msftclas
Copy link

msftclas commented May 13, 2019

CLA assistant check
All CLA requirements met.

Copy link
Member

@AnatoliB AnatoliB left a comment

Choose a reason for hiding this comment

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

Thank you, looks great. Please address the comment.

Update-AutomationAzureModulesForAccount.ps1 Outdated Show resolved Hide resolved
Update-AutomationAzureModulesForAccount.ps1 Show resolved Hide resolved
Update-AutomationAzureModulesForAccount.ps1 Outdated Show resolved Hide resolved
@ayoung012
Copy link
Contributor Author

@AnatoliB changes made as per requests

README.md Outdated Show resolved Hide resolved
@AnatoliB
Copy link
Member

Thank you, @ayoung012.
I've just tried to run this code and realized there is one more important issue to address. When 'Az' is passed as AzureModuleClass, this runbook upgrades Az modules, but it also upgrades the AzureRM modules present in the Automation account (and some AzureRM modules will always be there). Upgrading AzureRM modules will bring these modules to some PS sessions on some sandboxes for this account, which will cause problems. This may appear to work sometimes, but busy Automation accounts will definitely experience intermittent issues. So, when 'Az' is passed, this runbook should completely ignore AzureRM modules, and the other way around: ignore Az when 'AzureRM' is passed. You can check if these modules are tagged differently on the PS Gallery. If they are not, I would suggest simply filtering by name.
We are almost there, thank you again for your contribution! :-)

@ayoung012
Copy link
Contributor Author

They are not tagged differently on the PS gallery. It will have to be name filtered.

Is it possible to install the 'Az' module and 'AzureRM' to an automation account in its entirety? If so should these be filtered too?

I see the filter checking for the following:
Az.*
Az
AzureRM
AzureRM.*

Anything I have missed?

@AnatoliB
Copy link
Member

Is it possible to install the 'Az' module and 'AzureRM' to an automation account in its entirety? If so should these be filtered too?

This is not recommended but technically possible.

I see the filter checking for the following:
Az.*
Az
AzureRM
AzureRM.*

Looks good to me.

@ayoung012 ayoung012 force-pushed the master branch 2 times, most recently from 44f430a to 7610d38 Compare June 6, 2019 00:58
Copy link
Member

@AnatoliB AnatoliB left a comment

Choose a reason for hiding this comment

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

Thank you!

One more thing: this change breaks the unit tests. You can run the tests by invoking Invoke-Pester .\Tests. I suspect this happened because the tests are using FakeAzureModule as a fictional Azure module name, and the new logic ignores this module based on the name. Most likely, replacing FakeAzureModule with something like AzureRM.FakeModule in all the tests will fix the issue.

Ideally, it would also be great to add new tests verifying the Az vs. AzureRM logic, but I will not be insisting on that. However, we should not break the existing tests.

@mortenlerudjordet
Copy link

mortenlerudjordet commented Jun 10, 2019

If I can give a suggestion, instead of doing:
& $GetAutomationModule
an alternative is to let everything be and use Enable-AzureRmAlias instead.

As an example of how to support both Az and AzureRM without changing the function names from AzureRM to Az.

if((Get-Module -Name "Az.Accounts" -ListAvailable) -and (Get-Module -Name "Az.Automation" -ListAvailable) -and (Get-Module -Name "Az.Resources" -ListAvailable))
{
    $AccountsModule = Get-Module -Name Az.Accounts -ListAvailable
    $AutomationModule = Get-Module -Name Az.Automation -ListAvailable
    $ResourcesModule = Get-Module -Name Az.Resources -ListAvailable

    Write-Output -InputObject "Running Az.Account version: $($AccountsModule.Version)"
    Write-Output -InputObject "Running Az.Automation version: $($AutomationModule.Version)"
    Write-Output -InputObject "Running Az.Resources version: $($ResourcesModule.Version)"

    Import-Module -Name Az.Accounts, Az.Automation, Az.Resources -ErrorAction Continue -ErrorVariable oErr
    if($oErr)
    {
        Write-Error -Message "Failed to load needed modules for Runbook: Az.Accounts, Az.Automation,Az.Resources" -ErrorAction Continue
        throw "Check AA account for modules"
    }
    Write-Output -InputObject "Using Az modules to execute runbook"
    # This will negate the need to change syntax of AzureRM function names even if using Az modules
    Enable-AzureRmAlias
}
elseif((Get-Module -Name AzureRM.Profile -ListAvailable) -and (Get-Module -Name AzureRM.Automation -ListAvailable) -and (Get-Module -Name AzureRM.Resources -ListAvailable))
{
    $ProfileModule = Get-Module -Name AzureRM.Profile -ListAvailable
    $AutomationModule = Get-Module -Name AzureRM.Automation -ListAvailable
    $ResourcesModule = Get-Module -Name AzureRM.Resources -ListAvailable

    Write-Output -InputObject "Running AzureRM.Profile version: $($ProfileModule.Version)"
    Write-Output -InputObject "Running AzureRM.Automation version: $($AutomationModule.Version)"
    Write-Output -InputObject "Running AzureRM.Resources version: $($ResourcesModule.Version)"

    if( ([System.Version]$ProfileModule.Version -le [System.Version]"5.0.0") -and ([System.Version]$AutomationModule.Version -le [System.Version]"5.0.0") -and ([System.Version]$ResourcesModule.Version -le [System.Version]"5.0.0") )
    {
        Write-Error -Message "Manually update: AzureRM.Profile, AzureRM.Automation, AzureRM.Resources for first time usage through the portal" -ErrorAction Continue
        throw "Check AA account for modules"
    }
    else
    {
        Import-Module -Name AzureRM.Profile, AzureRM.Automation, AzureRM.Resources -ErrorAction Continue -ErrorVariable oErr
        if($oErr)
        {
            Write-Error -Message "Failed to load needed modules for Runbook: AzureRM.Profile, AzureRM.Automation,AzureRM.Resources" -ErrorAction Continue
            throw "Check AA account for modules"
        }
    }
    Write-Output -InputObject "Using AzureRM modules to execute runbook"
}
else
{
    Write-Error -Message "Did not find AzureRM or Az modules installed in Automation account" -ErrorAction Stop
}

An added bonus is that this will probably not break the existing tests

@AnatoliB
Copy link
Member

@mortenlerudjordet I like the idea of using Enable-AzureRmAlias in general, I just have one concern here: in Automation, runspaces can be reused between jobs, so these aliases will also be automatically applied to some (but not all!) other jobs. This is probably ok in most cases. I can imagine situations when this would create problems, but these situations are probably very rare, and these users can find workarounds.

@ayoung012 I'm ok either way: feel free to use or not use Enable-AzureRmAlias in this PR, I will approve it in any case. My personal preference would be to merge this PR first (after fixing the tests), and use Enable-AzureRmAlias in a separate PR, but I'll leave this up to you.

@AnatoliB
Copy link
Member

An added bonus is that this will probably not break the existing tests

However, this will probably not help with the current test break: as far as I understand, the tests are broken not because of using wrong cmdlets, but because of the new logic that intentioinially separates Az from AzureRM based on module names.

@mortenlerudjordet
Copy link

@AnatoliB If you execute Disable-AzureRmAlias at the end will this help with the reuse problem?

@AnatoliB
Copy link
Member

AnatoliB commented Jun 10, 2019

@mortenlerudjordet

If you execute Disable-AzureRmAlias at the end will this help with the reuse problem?

I don't see how this would help: the problem is that this command will bring the AzureRm aliases into the current PS runspace. Regardless of where it is invoked, the result will be the runspace will have these aliases left over. Now, this runspace may be used again for running any other job on this Automation account, and these new jobs may be affected. As an Automation user, you don't have direct control over runspace reuse.

But then again, I don't see any realistic and important scenario where the presence of the aliases would actually create a big problem. As soon as Az is loaded, the user is not supposed to use AzureRM cmdlets on this Automation account anyway, so the fact that AzureRM cmdlets are hidden by aliases should not actually hurt. And, if it does, they can always invoke Disable-AzureRmAlias as a workaround. So, no big problem, just something to be aware of.

Sorry, I misunderstood you, I thought you were talking about Enable-AzureRmAlias. Yes, invoking Disable-AzureRmAlias at the end should resolve this issue.

@ayoung012
Copy link
Contributor Author

ayoung012 commented Jun 10, 2019 via email

@AnatoliB
Copy link
Member

@ayoung012 I suspect the migration doc mentions removing AzureRM as a general good practice simply to make sure AzureRM modules are not loaded, not because there is any specific issue with enabling aliases. Unfortunately, we cannot completely remove AzureRM modules from Automation. You can check the cmdlet source code - it is open source - most likely it simply creates aliases, so it should not cause any problem.

@mortenlerudjordet
Copy link

mortenlerudjordet commented Jun 11, 2019

Only problem I had with the suggested use was that if you have a new AA account with the default AzureRM versions the import of AzureRM.profile, AzureRM.Automation and AzureRM.Resources will fail.
And therefore the update runbook will fail.

One will need to update them to the latest version "manually" (or using code that does not have any dependencies to the Azure modules). Though this more a problem with how old the AzureRM version that comes with a new AA account are.

The shared resources in use for executing runbooks needs to be updated with newer versions of AzureRM.

Copy link
Member

@AnatoliB AnatoliB left a comment

Choose a reason for hiding this comment

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

Thank you @ayoung012!

@AnatoliB AnatoliB merged commit f9f4e2f into microsoft:master Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants