feat: Add support for priorityClass and make updateStrategy configurable #189
Conversation
variables.tf
Outdated
| variable "create_priority_class" { | ||
| type = bool | ||
| description = "Specify whether or not to create a priority class for the agent daemonset." | ||
| default = true |
There was a problem hiding this comment.
Should this default to true? I'm guessing any user with a general use-case wouldn't need it.
setting this to true, is it your project specific requirement?
There was a problem hiding this comment.
In old module & chart, the daemonset had priority class enabled by default, see here.
Changed it to false
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
ocofaigh
left a comment
ocofaigh
left a comment
There was a problem hiding this comment.
Some comments:
- It would seem some of the new inputs can't be null. for those, please use
nullable = false. - Please use "monitoring agent" instead of sysdig in the variable descrptions.
variables.tf
Outdated
|
|
||
| variable "max_surge" { | ||
| type = string | ||
| description = "The maximum number of nodes that can have an extra DaemonSet pod during a rolling update. Accepts absolute number or percentage (e.g., '1' or '10%')." |
There was a problem hiding this comment.
The default value of this is null. What does that mean for the deployment? Please include that detail in the description 9same for DA descrption)
main.tf
Outdated
| ${line} | ||
| %{endfor~} | ||
| "createPriorityClass": ${var.create_priority_class} | ||
| "priorityClassName": ${var.priority_class_name == null ? "null" : var.priority_class_name} |
There was a problem hiding this comment.
Is "priorityClassName": "null" a supported helm value ? Should we use yaml templating here to simply omit priorityClassName from the yaml if its null?
variables.tf
Outdated
| } | ||
| } | ||
|
|
||
| variable "create_priority_class" { |
There was a problem hiding this comment.
Do we really need this boolean? Can't we just set it to true in the yaml if priority_class_name and priority_class_value have values? That would mean one less user input
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
ocofaigh
left a comment
ocofaigh
left a comment
There was a problem hiding this comment.
Few more comments on variable description updates - ensure they are done in both module and DA
|
/run pipeline |
1 similar comment
|
/run pipeline |
This reverts commit 853991a.
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
4 participants
Description
This PR make following changes:
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers