-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_kubernetes_cluster
- maintenance_window_*
added
#21760
azurerm_kubernetes_cluster
- maintenance_window_*
added
#21760
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @aristosvo. I agree with the design decisions you've made regarding separate blocks for each type as well as flattening the schedule properties within the block, definitely looks cleaner and reads easier within the code. I have some very minor suggestions about the log messages but overall this looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good - think just the docs are missing now.
Co-authored-by: stephybun <steph@hashicorp.com>
fb7091e
to
2ef2f75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @aristosvo LGTM 💾
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #21722
TODO
Considerations
(especially 2 and 3)Considerations
1 or multiple blocks:
The default maintenance window type (the "legacy" type, according to the docs) has a different model compared to the other two types. Supporting these 2 different models in one block type is a bit messy from a user perspective, as we cannot validate it as extensively as when we differentiate (option 2). Option 1 will result in a bit more confusion and questions, option 2 is more clear and directing.
In the new maintenance window type there are multiple frequencies possible (Daily, Weekly, AbsoluteMonthly and RelativeMonthly), which in itself are already adding a lot of parameters and confusion. That adds up to the before mentioned preference for option 2.
This means that I've chosen option 2 for now 🙈
Frequency schedules as separate blocks vs flat structure
I'd prefer option 2, as is makes validation better and mimics the API more closely.
Date, time: what are the standards?
We do have standards for date time, but what to use in case of date and time? I feel like it is a bit mixed across the provider? I've modeled dates now as datetime, but that doesn't sound like a great solution in the long run