Skip to content

Add support for maintenance_window#256

Merged
lonegunmanb merged 7 commits intoAzure:masterfrom
lonegunmanb:feat-255
Sep 30, 2022
Merged

Add support for maintenance_window#256
lonegunmanb merged 7 commits intoAzure:masterfrom
lonegunmanb:feat-255

Conversation

@lonegunmanb
Copy link
Copy Markdown
Member

This pr fix #255 by add a new variable named maintenance_window.

We also set this config in two example folders.

@lonegunmanb lonegunmanb requested review from jiaweitao001 and zioproto and removed request for zioproto September 26, 2022 09:11
@zioproto
Copy link
Copy Markdown
Contributor

There was an earlier PR #133 for this same feature.

The PRs are almost identical, except that #133 introduces multiple variables and this one uses a single maintenance_window object.

All comments done previously in #133 are addressed already by changes in this one.

@github-actions
Copy link
Copy Markdown
Contributor

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

Comment thread variables.tf Outdated

variable "maintenance_window" {
type = object({
allowed = optional(list(object({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I dont understand this failure in the CI:
https://github.com/Azure/terraform-azurerm-aks/actions/runs/3134168836/jobs/5088466915#step:10:37

Is the prepr-check failing because of optional ? I understand now that this is supported in Terraform 1.3.

I see that we install terraform here:
https://github.com/Azure/terraform-azurerm-aks/blob/master/.github/workflows/pr-check.yaml#L16

this should install always the latest Terraform version ? How do we check which exact Terraform version we are using in the CI ?

thanks

@zioproto
Copy link
Copy Markdown
Contributor

If we introduce the use of optional that requires Terraform 1.3, this is not a breaking change, but I think it requires a major version bump of the module to 7.0.0.

This is introducing a new feature, so this PR should not break anything because the feature was not supported before.

However, if a customer on Terraform 1.2 cant upgrade to Terraform 1.3 for whatever reason, the customer should be able to continue using any version 6.x.x of this module.

Please let me know if it makes sense, thanks

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

Comment thread main.tf
dynamic "not_allowed" {
for_each = var.maintenance_window.not_allowed

content {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens here if the user enters a value in the past ? Only time windows in the future make sense here. A value in the past here would be useless, so I expect the API to raise an error.

Should we add a Terraform conditional checking ?

There are good examples here to make conditionals with times:
https://www.terraform.io/language/functions/timecmp#examples

I am thinking of something like:

  lifecycle {
    precondition {
      condition     = timecmp(timestamp(), not_allowed.value.end ) < 0
      error_message = "The specified maintenance window exception is in the past"
    }
  }

I am looking here:
https://learn.microsoft.com/en-us/azure/aks/planned-maintenance

but I can't figure out if a value in the past is accepted by the API.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks @zioproto for the review. I tend not to add this precondition since the provider didn't limit this. Maybe the caller just generate this not_allowed list by code, or read it from their CMDB, maybe they just don't care whether if the parameters contain a past time or not.

@github-actions
Copy link
Copy Markdown
Contributor

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

@lonegunmanb lonegunmanb temporarily deployed to acctests September 30, 2022 03:28 Inactive
Copy link
Copy Markdown
Contributor

@zioproto zioproto left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Support for maintenance_window

2 participants