Open Service Mesh addon#132
Conversation
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
| open_service_mesh { | ||
| enabled = var.enable_open_service_mesh | ||
| } | ||
|
|
There was a problem hiding this comment.
Looking at the documentation this is not a block:
https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/kubernetes_cluster#open_service_mesh_enabled
It should be a oneliner:
open_service_mesh_enabled = var.enable_open_service_mesh
There was a problem hiding this comment.
I actually tested this and it works with provider registry.terraform.io/hashicorp/azurerm v2.99.0. The change I proposed will be needed once the module will use the version 3 of the provider.
There was a problem hiding this comment.
@zioproto you're right. For now this module works with provider 2.x so this patch is correct, but we're close to merge a pr that upgrade provider to 3.x so I'm not sure whether we should continue work on 2.x. My thought is we upgrade to 3.x first then let's see what we can do for this pr.
| } | ||
|
|
||
| open_service_mesh_enabled = var.enable_open_service_mesh | ||
|
|
There was a problem hiding this comment.
@nlamirault you introduced a line with empty spaces at line 102. We are going to introduce very soon a CI that will lint the code, and this file will not pass the linting. Could you please remove the empty spaces ?
To avoid this pain in the future you can set your editor to automatically trim empty spaces at the end of lines when you save the files. Thank you
There was a problem hiding this comment.
Sorry 👎 I made the fix using Github UI ... I fix that asap.
There was a problem hiding this comment.
Hi @zioproto agreed but I think we can accept this pr for this time since we'll submit a single pr to fix all the naming issues and style issues.
lonegunmanb
left a comment
lonegunmanb
left a comment
There was a problem hiding this comment.
Hi @nlamirault these changes LGTM but there're some conflicts because we've accepted some prs, would you please solve these conflicts so I can merge this pr? Thanks!
lonegunmanb
left a comment
lonegunmanb
left a comment
There was a problem hiding this comment.
Almost LGTM but two tiny issues, would you consider make change? Thanks @nlamirault !
| } | ||
|
|
||
| variable "enable_open_service_mesh" { | ||
| description = "Enable Open Service Mesh Addon." |
There was a problem hiding this comment.
Can we use the same description as provider's document?
| variable "enable_open_service_mesh" { | ||
| description = "Enable Open Service Mesh Addon." | ||
| type = bool | ||
| default = false |
There was a problem hiding this comment.
Would the default value null be better? A false default value will cause changes in plan when the users upgrade module's value.
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
lonegunmanb
left a comment
lonegunmanb
left a comment
There was a problem hiding this comment.
Thanks @nlamirault , LGTM
3 participants
This PR add support for Open Service Mesh for AKS