Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Output VMSS cloud config settings. #3610

Merged
merged 2 commits into from
Aug 7, 2018

Conversation

JunSun17
Copy link
Collaborator

@JunSun17 JunSun17 commented Aug 3, 2018

Output VMSS cloud config settings for AKS to consume.

@JunSun17 JunSun17 requested review from jackfrancis and weinong August 3, 2018 18:15
@acs-bot
Copy link

acs-bot commented Aug 3, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JunSun17
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: brendandburns

If they are not already assigned, you can assign the PR to them by writing /assign @brendandburns in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ghost ghost assigned JunSun17 Aug 3, 2018
@ghost ghost added the in progress label Aug 3, 2018
@acs-bot acs-bot added the size/S label Aug 3, 2018
@codecov
Copy link

codecov bot commented Aug 3, 2018

Codecov Report

Merging #3610 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3610      +/-   ##
==========================================
+ Coverage    55.4%   55.45%   +0.04%     
==========================================
  Files         107      107              
  Lines       16210    16210              
==========================================
+ Hits         8981     8989       +8     
+ Misses       6453     6445       -8     
  Partials      776      776

@jackfrancis
Copy link
Member

@JunSun17 What's the functional purpose of this change?

"type": "string",
"value": "[variables('primaryScaleSetName')]"
},
"vmType": {
Copy link
Contributor

Choose a reason for hiding this comment

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

we really dont need this

Copy link
Collaborator Author

@JunSun17 JunSun17 Aug 4, 2018

Choose a reason for hiding this comment

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

This change is originally target to pass the cloud config settings of vmType and primaryScaleSetName to AKS (in the way of exposing it to ARM output). This is the way we used to pass all cloud provider config settings.

I will see if we can build these strings in AKS for its cloud provider config.

@acs-bot acs-bot added size/XS and removed size/S labels Aug 6, 2018
@@ -359,11 +359,11 @@
"nsgID": "[resourceId('Microsoft.Network/networkSecurityGroups',variables('nsgName'))]",
{{if not AnyAgentUsesVirtualMachineScaleSets}}
"primaryAvailabilitySetName": "[concat('{{ (index .AgentPoolProfiles 0).Name }}-availabilitySet-',variables('nameSuffix'))]",
"primaryScaleSetName": "''",
"primaryScaleSetName": "",
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of these changes (from literal '' to empty string)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it is set to "''", it will not be an empty string in the output. the output will be ''. We need an empty string as the result.

@jackfrancis jackfrancis merged commit 46162bb into Azure:master Aug 7, 2018
@ghost ghost removed the in progress label Aug 7, 2018
jackfrancis pushed a commit that referenced this pull request Aug 7, 2018
@JunSun17 JunSun17 deleted the jusu-vmss-cloud-provider branch August 23, 2018 21:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants