feat(storage_profile): add support for CSI arguments #282

aescrob · 2022-12-13T13:02:25Z

Describe your changes

Since settings are supported by terraform-azurerm-provider from v3.35.0 on, allowing to disable CSI driver one by one.

Issue number

#000

Checklist before requesting a review

The pr title can be used to describe what this pr did in CHANGELOG.md file
I have executed pre-commit on my machine
I have passed pr-check on my machine

Thanks for your cooperation!

lonegunmanb

Thanks @aescrob for opening this pr! We have a CI pipeline in this repo, would you please follow the instruction in the readme, run pre-commit job on you machine and try again? Thanks.

lonegunmanb · 2022-12-14T00:23:57Z

variables.tf

@@ -237,7 +237,7 @@ variable "load_balancer_profile_managed_outbound_ip_count" {

 variable "load_balancer_profile_managed_outbound_ipv6_count" {
  type        = number
-  description = "(Optional) The desired number of IPv6 outbound IPs created and managed by Azure for the cluster load balancer. Must be in the range of `1` to `100` (inclusive). The default value is `0` for single-stack and `1` for dual-stack. Note: managed_outbound_ipv6_count requires dual-stack networking. To enable dual-stack networking the Preview Feature Microsoft.ContainerService/AKS-EnableDualStack needs to be enabled and the Resource Provider re-registered, see the documentation for more information. https://learn.microsoft.com/en-us/azure/aks/configure-kubenet-dual-stack?tabs=azure-cli%2Ckubectl#register-the-aks-enabledualstack-preview-feature"
+  description = "(Optional) The desired number of IPv6 outbound IPs created and managed by Azure for the cluster load balancer. Must be in the range of `1` to `100` (inclusive). The default value is `0` for single-stack and `1` for dual-stack. Note: managed_outbound_ipv6_count requires dual-stack networking. To enable dual-stack networking the Preview Feature Microsoft.ContainerService/AKS-EnableDualStack needs to be enabled and the Resource Provider re-registered, see the documentation for more information. https://learn.microsoft.com/en-us/azure/aks/configure-kubenet-dual-stack?tabs=azure-cli%!C(MISSING)kubectl#register-the-aks-enabledualstack-preview-feature"


I think we've got a typo in this description.

lonegunmanb · 2022-12-14T00:28:38Z

main.tf

@@ -219,6 +219,13 @@ resource "azurerm_kubernetes_cluster" "main" {
      client_secret = var.client_secret
    }
  }
+  storage_profile {


Could we make this block a dynamic block and add a new toggle variable so we can turn it on and off like we've done for aci_connector_linux?

lonegunmanb · 2022-12-14T00:40:19Z

variables.tf

+variable "storage_profile_blob_driver_enabled" {
+  type        = bool
+  description = "(Optional) Is the Blob CSI driver enabled? Defaults to false"
+  default     = null


According to the schema, the default should be false.

lonegunmanb · 2022-12-14T00:40:43Z

variables.tf

+variable "storage_profile_disk_driver_enabled" {
+  type        = bool
+  description = "(Optional) Is the Disk CSI driver enabled? Defaults to true"
+  default     = null


According to the schema, the default should be true.

lonegunmanb · 2022-12-14T00:41:25Z

variables.tf

+variable "storage_profile_disk_driver_version" {
+  type        = string
+  description = "(Optional) Disk CSI Driver version to be used. Possible values are v1 and v2. Defaults to v1."
+  default     = null


According to the schema, the default should be "v1".

lonegunmanb · 2022-12-14T00:42:01Z

variables.tf

+variable "storage_profile_file_driver_enabled" {
+  type        = bool
+  description = "(Optional) Is the File CSI driver enabled? Defaults to true"
+  default     = null


According to the schema, the default should be true.

lonegunmanb · 2022-12-14T00:42:35Z

variables.tf

+variable "storage_profile_snapshot_controller_enabled" {
+  type        = bool
+  description = "(Optional) Is the Snapshot Controller enabled? Defaults to true"
+  default     = null


According to the schema, the default should be true.

lonegunmanb

Apology for another change request @aescrob, I wasn't able to figure them out at the first review.

Since we use terraform-docs to generate document from variable and output descriptions, would you please help us by back-quoting values inside the description? Thanks!

And one more thing, our pipeline was broken because of Checkov has added some new policies, so please wait until #281 was merged then rebase to the latest master branch, thanks for your understanding and cooperation!

lonegunmanb · 2022-12-15T01:55:08Z

variables.tf

@@ -530,6 +530,42 @@ variable "sku_tier" {
  default     = "Free"
 }

+variable "storage_profile_blob_driver_enabled" {
+  type        = bool
+  description = "(Optional) Is the Blob CSI driver enabled? Defaults to false"


Would you please back quote the value inside these descriptions since we'll generate terraform document from them? Thanks!

description = "(Optional) Is the Blob CSI driver enabled? Defaults to `false`"

lonegunmanb · 2022-12-15T01:55:30Z

variables.tf

+
+variable "storage_profile_disk_driver_enabled" {
+  type        = bool
+  description = "(Optional) Is the Disk CSI driver enabled? Defaults to true"


description = "(Optional) Is the Disk CSI driver enabled? Defaults to `true`"

lonegunmanb · 2022-12-15T01:56:00Z

variables.tf

+
+variable "storage_profile_disk_driver_version" {
+  type        = string
+  description = "(Optional) Disk CSI Driver version to be used. Possible values are v1 and v2. Defaults to v1."


description = "(Optional) Disk CSI Driver version to be used. Possible values are v1 and v2. Defaults to v1."

lonegunmanb · 2022-12-15T01:56:25Z

variables.tf

+
+variable "storage_profile_file_driver_enabled" {
+  type        = bool
+  description = "(Optional) Is the File CSI driver enabled? Defaults to true"


description = "(Optional) Is the File CSI driver enabled? Defaults to `true`"

lonegunmanb · 2022-12-15T01:59:54Z

variables.tf

+
+variable "storage_profile_snapshot_controller_enabled" {
+  type        = bool
+  description = "(Optional) Is the Snapshot Controller enabled? Defaults to true"


description = "(Optional) Is the Snapshot Controller enabled? Defaults to `true`"

lonegunmanb · 2022-12-15T02:04:39Z

variables.tf

+variable "storage_profile_enabled" {
+  description = "Enable storage profile"
+  type        = bool
+  default     = false


Could we add nullable = false here?

lonegunmanb · 2022-12-15T06:03:29Z

Hi @aescrob, since the master branch now should have been fixed, would you please rebase to the latest master branch and try again? Thanks.

aescrob · 2022-12-15T07:46:01Z

@aescrob please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

Contributor License Agreement

@microsoft-github-policy-service agree [company="swisspost"]

aescrob · 2022-12-15T07:47:24Z

@aescrob please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

Contributor License Agreement

@microsoft-github-policy-service agree company="swisspost"

lonegunmanb

Hi @aescrob, this pr has passed the e2e test and almost LGTM, only one extra commit with declaring var.storage_profile_enabled's nullable = false would make this pr been merged, thanks for your understanding.

lonegunmanb · 2022-12-22T01:36:33Z

Hello @aescrob, I'm sorry we've met an issue on our pipeline so we have to upgrade the Github action yaml files in this repo, would you please merge your branch with the latest master branch so we can run the e2e test? Apology for the inconvenience, we're still refactoring the pipeline.

aescrob · 2022-12-23T11:14:59Z

Oh...i am so sad to not made it into v6.3.0.
Anyway, thank you @lonegunmanb for your feedbacks

lonegunmanb · 2022-12-25T02:55:27Z

Oh...i am so sad to not made it into v6.3.0. Anyway, thank you @lonegunmanb for your feedbacks

No worries @aescrob , I'll release a new version for you once this pr has been merged.

lonegunmanb

Thanks @aescrob, LGTM! 🚀

lonegunmanb requested changes Dec 14, 2022

View reviewed changes

aescrob requested a review from lonegunmanb December 14, 2022 10:50

lonegunmanb requested changes Dec 15, 2022

View reviewed changes

aescrob requested a review from lonegunmanb December 15, 2022 07:49

aescrob temporarily deployed to acctests December 15, 2022 12:46 — with GitHub Actions Inactive

lonegunmanb requested changes Dec 16, 2022

View reviewed changes

feat(storage_profile): add support for CSI arguments

1454d40

aescrob requested a review from lonegunmanb December 16, 2022 07:47

aescrob had a problem deploying to acctests December 19, 2022 04:44 — with GitHub Actions Error

aescrob had a problem deploying to acctests December 19, 2022 11:10 — with GitHub Actions Failure

aescrob had a problem deploying to acctests December 21, 2022 01:32 — with GitHub Actions Error

Merge commit '6b365c7' into storage_profile

d132f9a

aescrob had a problem deploying to acctests December 25, 2022 02:55 — with GitHub Actions Error

aescrob temporarily deployed to acctests December 25, 2022 12:26 — with GitHub Actions Inactive

lonegunmanb approved these changes Dec 26, 2022

View reviewed changes

lonegunmanb merged commit 16aac71 into Azure:master Dec 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage_profile): add support for CSI arguments #282

feat(storage_profile): add support for CSI arguments #282

aescrob commented Dec 13, 2022 •

edited

Loading

lonegunmanb left a comment

lonegunmanb Dec 14, 2022

aescrob Dec 14, 2022

lonegunmanb Dec 14, 2022

aescrob Dec 14, 2022

lonegunmanb Dec 14, 2022

aescrob Dec 14, 2022

lonegunmanb Dec 14, 2022

aescrob Dec 14, 2022

lonegunmanb Dec 14, 2022

aescrob Dec 14, 2022

lonegunmanb Dec 14, 2022

aescrob Dec 14, 2022

lonegunmanb Dec 14, 2022

aescrob Dec 14, 2022

lonegunmanb left a comment

lonegunmanb Dec 15, 2022

lonegunmanb Dec 15, 2022

lonegunmanb Dec 15, 2022

lonegunmanb Dec 15, 2022

lonegunmanb Dec 15, 2022

lonegunmanb Dec 15, 2022

lonegunmanb commented Dec 15, 2022 •

edited

Loading

aescrob commented Dec 15, 2022

aescrob commented Dec 15, 2022

lonegunmanb left a comment

lonegunmanb commented Dec 22, 2022

aescrob commented Dec 23, 2022

lonegunmanb commented Dec 25, 2022

lonegunmanb left a comment

feat(storage_profile): add support for CSI arguments #282

feat(storage_profile): add support for CSI arguments #282

Conversation

aescrob commented Dec 13, 2022 • edited Loading

Describe your changes

Issue number

Checklist before requesting a review

lonegunmanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lonegunmanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lonegunmanb commented Dec 15, 2022 • edited Loading

aescrob commented Dec 15, 2022

aescrob commented Dec 15, 2022

lonegunmanb left a comment

Choose a reason for hiding this comment

lonegunmanb commented Dec 22, 2022

aescrob commented Dec 23, 2022

lonegunmanb commented Dec 25, 2022

lonegunmanb left a comment

Choose a reason for hiding this comment

aescrob commented Dec 13, 2022 •

edited

Loading

lonegunmanb commented Dec 15, 2022 •

edited

Loading