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

AzureEnvironmentBuilder: VM max name length and fix for Azure PowerShell 6.0.0 #890

Merged
merged 3 commits into from Sep 26, 2018

Conversation

jensotto
Copy link
Contributor

@jensotto jensotto commented Sep 22, 2018

Pull Request (PR) description

Virtual Machines can have names of maximum 15 characters. Previously the VM name was concatenated using the resource group name and a postfix of up to 4 characters. This would leave only 11 characters for the resource group name which would only fail upon VM creation. Now the VMs have a fixed name (VM-dc/VM-sql/VM-sp)
Changed Find-AzureRmResource to Get-AzureRmResource as Find-AzureRmResource has been removed in Azure PowerShell 6.0.0
https://github.com/Azure/azure-powershell/releases/tag/v6.0.0-May2018
"Merge Get- and Find- functionality in Get-AzureRmResource"

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in README.md in the resource folder.
  • Resource parameter descriptions added/updated in schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

Shortened VM names as max limit is 15 characters
@codecov-io
Copy link

codecov-io commented Sep 22, 2018

Codecov Report

Merging #890 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##             dev    #890   +/-   ##
=====================================
  Coverage     91%     91%           
=====================================
  Files        113     113           
  Lines      11112   11112           
  Branches       1       1           
=====================================
  Hits       10177   10177           
  Misses       934     934           
  Partials       1       1

@johlju
Copy link
Member

johlju commented Sep 25, 2018

@jensotto there was other PR's merged into dev, so your working branch is out of date compared to dev. Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history). If you don't know how to rebase your local dev and working branch, please look at how to Resolve merge conflicts.
Let us know if you need any assistance.

@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Sep 25, 2018
@johlju johlju requested a review from ykuijs September 25, 2018 14:32
@jensotto
Copy link
Contributor Author

@johlju done

Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

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

One small comment

FYI: These templates are used for the integration tests. These were added a while back, but never implemented because we couldn't find a good way of implementing these tests in an automated fashion.

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @jensotto and @ykuijs)


Tests/Integration/Azure/AzureEnvironmentBuilder.psm1, line 134 at r1 (raw file):

    # Get keys for software storage
    $storageAccount = Get-AzureRmResource -ResourceName $SoftwareStorageAccountName

Does this cmdlet also exists in pre-v6.0?

Since SharePoint requires Windows PowerShell, we cannot assume that everybody is using PS6. So if this cmdlet doesn't exist in PS5.1, we should implement a check.

Copy link
Contributor Author

@jensotto jensotto left a comment

Choose a reason for hiding this comment

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

I was just following the Wiki page Creating an Azure development environment to set up a new development environment in Azure. I have several Hyper-V environments on my laptop, but wanted to set it up on Azure. I never intended to use it for integration tests.

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @jensotto)


Tests/Integration/Azure/AzureEnvironmentBuilder.psm1, line 134 at r1 (raw file):

Previously, ykuijs (Yorick Kuijs) wrote…

Does this cmdlet also exists in pre-v6.0?

Since SharePoint requires Windows PowerShell, we cannot assume that everybody is using PS6. So if this cmdlet doesn't exist in PS5.1, we should implement a check.

It is not a cmdlet in PS5.1 or PS6 it is a cmdlet in Azure PowerShell 6.0.0. It has nothing to do with SharePoint and the instructions on the Wiki page for installing Azure PowerShell installs the lates version of Azure PowerShell.

Copy link
Contributor Author

@jensotto jensotto left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @ykuijs)


Tests/Integration/Azure/AzureEnvironmentBuilder.psm1, line 134 at r1 (raw file):

Previously, jensotto (Jens Otto Hatlevold) wrote…

It is not a cmdlet in PS5.1 or PS6 it is a cmdlet in Azure PowerShell 6.0.0. It has nothing to do with SharePoint and the instructions on the Wiki page for installing Azure PowerShell installs the lates version of Azure PowerShell.

Done.

Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


Tests/Integration/Azure/AzureEnvironmentBuilder.psm1, line 134 at r1 (raw file):

Previously, jensotto (Jens Otto Hatlevold) wrote…

Done.

Ok perfect, when I read 6.0 I automatically assume PS6.0.....my bad ;-)

@ykuijs ykuijs merged commit ac22dc8 into dsccommunity:dev Sep 26, 2018
@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Sep 26, 2018
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.

None yet

4 participants