Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintenance window support #133

Closed
wants to merge 9 commits into from

Conversation

nlamirault
Copy link
Contributor

Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
@nlamirault nlamirault changed the title Add: maintenance window support Maintenance window support Dec 2, 2021
@nlamirault
Copy link
Contributor Author

@jcorioland Any news for this feature ?

@jcorioland
Copy link
Contributor

@nlamirault I am contributor on this project, but I can't review/merge PR, sorry.

@nlamirault
Copy link
Contributor Author

ok @jcorioland .
Project seems not maintained ? Last commit is yours (2021/05/27)

@lonegunmanb
Copy link
Member

ok @jcorioland . Project seems not maintained ? Last commit is yours (2021/05/27)

Hi @nlamirault , @jcorioland was a contributor and his's pr was accepted and merged by the project's owner.

@nlamirault
Copy link
Contributor Author

Could you review this PR @yupwei68 please ?

@lonegunmanb
Copy link
Member

Hello @nlamirault , thanks for your pr! I'd like to merge this pr, but now we have conflicts to resolve, and I've left some comments, would you please take a look and push again? Thanks!

main.tf Outdated
@@ -84,6 +84,24 @@ resource "azurerm_kubernetes_cluster" "main" {
}
}

maintenance_window {
dynamic "allowed" {
for_each = var.enable_maintenance_window == true ? var.maintenance_allowed : []
Copy link
Member

Choose a reason for hiding this comment

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

Would the following expression be better?

for_each = var.enable_maintenance_window ? var.maintenance_allowed : []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

main.tf Outdated
}

dynamic "not_allowed" {
for_each = var.enable_maintenance_window == true ? var.maintenance_not_allowed : []
Copy link
Member

Choose a reason for hiding this comment

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

And this line too:

for_each = var.enable_maintenance_window ? var.maintenance_not_allowed : []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

variables.tf Outdated
description = "Days and hours when maintenance is allowed"
type = list(object({
day = string
hours = list(string)
Copy link
Member

Choose a reason for hiding this comment

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

Since the hours is number (doc), would the type list(number) be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Hi, I've tested the code with fixture in the test folder, and the following error occurred:

│ Error: Missing required argument
│
│   with module.aks.azurerm_kubernetes_cluster.main,
│   on ..\..\main.tf line 88, in resource "azurerm_kubernetes_cluster" "main":
│   88:   maintenance_window {
│
│ "maintenance_window.0.allowed": one of `maintenance_window.0.allowed,maintenance_window.0.not_allowed` must be specified
╵
╷
│ Error: Missing required argument
│
│   with module.aks.azurerm_kubernetes_cluster.main,
│   on ..\..\main.tf line 88, in resource "azurerm_kubernetes_cluster" "main":
│   88:   maintenance_window {
│
│ "maintenance_window.0.not_allowed": one of `maintenance_window.0.allowed,maintenance_window.0.not_allowed` must be specified
╵

The enable_maintenance_window default value false will emit an empty maintenance_window block which is not valid. Would the following code be better?:

dynamic "maintenance_window" {
    for_each = var.enable_maintenance_window ? ["maintenance_window"] : []
    content {
      dynamic "allowed" {
        for_each = var.maintenance_allowed
        content {
          day   = allowed.value.day
          hours = allowed.value.hours
        }
      }

      dynamic "not_allowed" {
        for_each = var.maintenance_not_allowed
        content {
          start = not_allowed.value.start
          end   = not_allowed.value.end
        }
      }
    }
  }

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #241, THIS PR WILL BE UPDATED.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR WILL BE UPDATED.

1 similar comment
@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR WILL BE UPDATED.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

1 similar comment
@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #230, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

1 similar comment
@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #253, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #249, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #245, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@zioproto
Copy link
Collaborator

@nlamirault this PR does not merge anymore in the current master. Please rebase this work asap or look into the proposed #256

@lonegunmanb
Copy link
Member

lonegunmanb commented Sep 27, 2022

That's ok @zioproto, I'm working on a new pr for this feature, I'll submit it when it's ready.

Btw, in my new pr, I'd like to use optional feature introduced by Terraform 1.3, which requires upgrading on Terraform Core's restriction to >= 1.3. Do you think such a change is a breaking change? If so, I'd like to defer this pr to our next new major release, maybe after we solve all the remaining prs.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@zioproto
Copy link
Collaborator

Let's keep the discussion in the thread on PR #256

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #259, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #262, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #260, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #251, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #256, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@github-actions
Copy link
Contributor

MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI.

@lonegunmanb
Copy link
Member

Thanks @nlamirault for open this pr, I'm closing it in favor of #256 has brought us this feature. Thanks for your contribution to this module.

@nlamirault
Copy link
Contributor Author

@lonegunmanb 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants