Skip to content

Comments

VM OrchestrationMode: PlatformFaultDomainCount in New-AzVmss and VmssId in New-AzVm#12871

Merged
markcowl merged 13 commits intomasterfrom
SandidoPFDCandVmssID
Oct 7, 2020
Merged

VM OrchestrationMode: PlatformFaultDomainCount in New-AzVmss and VmssId in New-AzVm#12871
markcowl merged 13 commits intomasterfrom
SandidoPFDCandVmssID

Conversation

@Sandido
Copy link
Contributor

@Sandido Sandido commented Sep 5, 2020

Description

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@Sandido Sandido marked this pull request as ready for review September 23, 2020 01:10
@msJinLei msJinLei added the Contains Breaking Change This PR contains breaking change label Sep 23, 2020
@msJinLei
Copy link
Contributor

@Sandido could you rebase to latest the master to suppress the test timeout?

@Sandido Sandido changed the title PlatformFaultDomainCount in New-AzVmss and VmssId in New-AzVm VM OrchestrationMode: PlatformFaultDomainCount in New-AzVmss and VmssId in New-AzVm Sep 24, 2020
@Sandido Sandido requested a review from msJinLei September 25, 2020 18:30
@msJinLei
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

msJinLei
msJinLei previously approved these changes Sep 28, 2020
@isra-fel isra-fel self-assigned this Sep 29, 2020
$vm = new-azvm -resourcegroupname $rgname -location $loc -name $vmname -credential $cred -domainnamelabel $domainName -vmssid $VmssWithoutVmProfile.id

Assert-AreEqual $VmssWithoutVmProfile.id $vm.virtualmachinescaleset.id

Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to view the instance view of vmss to see this vm added to the vmss?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grizzlytheodore , No. This functionality does not exist in PS and CLI due to how the feature is setup on the server side.

[Parameter(ParameterSetName = SimpleParameterSet, Mandatory = false,
ValueFromPipelineByPropertyName = true,
HelpMessage = "Fault Domain count for each placement group.")]
public int? PlatformFaultDomainCount { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

i believe '?' is not necessary here to be able to pass this parameter as null. Should behave the same when the variable type is 'int', check 'maxprice' parameter. I

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grizzlytheodore , having the parameter as the nullable int type (int?) removes the need to cast it as a nullable int in the resourceGroup.CreateVirtualMachineScaleSetConfig() call.
MaxPrice does have this cast (double?), so having the parameter int? removes the need for this cast.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Sandido I see. I think it's better to stay consistent on where the parameter is being casted as a nullable variable. Not only in this cmdlet, the variables are casted as a nullable variable in CreateVirtualMachineScaleSetConfig(), but other cmdlets follow the similar structure where '?' is left out at the parameter declaration, and later is casted as nullable when it is being checked with isParameter() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grizzlytheodore , I changed it as you suggested.

[Parameter(ParameterSetName = SimpleParameterSet, Mandatory = false)]
[Parameter(ParameterSetName = DiskFileParameterSet, Mandatory = false)]
public string VmssId { get; set; }

Copy link
Contributor

Choose a reason for hiding this comment

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

should we add vmssId parameter to new-azvmConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grizzlytheodore , it is already there, the -vmssId parameter.

@markcowl markcowl merged commit d56471f into master Oct 7, 2020
@Sandido Sandido deleted the SandidoPFDCandVmssID branch June 22, 2021 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Contains Breaking Change This PR contains breaking change Waiting for CI :shipit:

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants