feat: extend NodePool schema and CreateResourceGroup with optional fields#1196
feat: extend NodePool schema and CreateResourceGroup with optional fields#1196xinWeiWei24 wants to merge 2 commits into
Conversation
…ields Add vnetSubnetId and osSKU optional fields to NodePool schema, and tags optional parameter to CreateResourceGroup. Both default to empty/omitted for backwards compatibility with existing pipelines. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
xinWeiWei24
requested review from
liyu-ma,
vittoriasalim and
wonderyl
as code owners
| count: int | ||
| taintPrefix: str = "" | ||
| labels: str = "" | ||
| vnetSubnetId: str = "" |
There was a problem hiding this comment.
Is it better use str? type which has a default value None
| vnetSubnetId?: str | ||
| osSKU?: str | ||
|
|
||
| CreateNodePool = lambda serviceConnection: str, cluster: str, resourceGroup: str, subscription: str, pool: NodePool, noWait: bool = False -> steps.Step { |
There was a problem hiding this comment.
The list of parameters becomes longer and longer. I wonder if it makes sense to let users to write a az aks command directly instead? The wrapper is only meaningful if it simplify things. now it's just pass through everything.
There was a problem hiding this comment.
I feel it is ok as long as we don't add more params to CreateNodePool signature. We need schema NodePool to represent CL2 pool / worker pool and pass around. Users will need to configure/enrich NodePool for their use cases. I learnt you can do something like below which does not feel complicated:
planned = NodePool {
sku = "Standard_D4"
count = 3
}
configured = NodePool {**planned, vnetSubnetId = "my_subnet_id"}
|
|
||
| CreateResourceGroup = lambda serviceConnection: str, name: str, location: str, subscription: str -> steps.Step { | ||
| CreateResourceGroup = lambda serviceConnection: str, name: str, location: str, subscription: str, tags: str = "" -> steps.Step { | ||
| tagsFlag = " \\\n --tags ${tags}" if tags else "" |
There was a problem hiding this comment.
Normally it's called an argument.
$ az aks create -h
The behavior of this command has been altered by the following extension: aks-preview
Command
az aks create : Create a new managed Kubernetes cluster.
Arguments
--name -n [Required] : Name of the managed
| az group create \\ | ||
| --name "${name}" \\ | ||
| --location "${location}" \\ | ||
| --subscription "${subscription}"${tagsFlag} |
There was a problem hiding this comment.
nit: I feel it's more easier to read when you do
az group create \\
--name "${name}" \\
--location "${location}" \\
--subscription "${subscription}" \\n
${"--tags " + tags if tags else ""}
Even though it will end up have a training \n
| taintPrefix: str = "" | ||
| labels: str = "" | ||
| vnetSubnetId?: str | ||
| osSKU?: str |
There was a problem hiding this comment.
btw what is difference between sku and osSKU? I added sku param to represent the vm size for the node
3 participants
Add vnetSubnetId and osSKU optional fields to NodePool schema, and tags optional parameter to CreateResourceGroup. Both default to empty/omitted for backwards compatibility with existing pipelines.