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

kubernetes_cluster kubernetes_cluster_node_pool - message_of_day scale_down_mode workload_runtime load_balancer_profile/managed_outbound_ipv6_count #16741

Conversation

qiqingzhang
Copy link
Contributor

public pr for kubernetes_cluster kubernetes_cluster_node_pool - message_of_day scale_down_mode workload_runtime load_balancer_profile/managed_outbound_ipv6_count

test result after update:

1:TestAccKubernetesCluster_nodePoolOther
image

2:TestAccKubernetesCluster_scaleDownMode:
image

3:TestAccKubernetesClusterNodePool_other:
image

Copy link
Member

@stephybun stephybun left a 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 @qiqingzhang I left some suggestions in-line regarding the property names and documentation. Once they're fixed up we can take another look over this!

"message_of_day": {
Type: pluginsdk.TypeString,
Optional: true,
ForceNew: true,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be ForceNew since we're checking for changes and updating the value below?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can't be updated, I'll remove the codes checking for changes and updating.

@@ -155,6 +156,13 @@ func resourceKubernetesClusterNodePool() *pluginsdk.Resource {
ForceNew: true,
},

"message_of_day": {
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be clearer as message_of_the_day

@@ -115,6 +116,13 @@ func SchemaDefaultNodePool() *pluginsdk.Schema {
ForceNew: true,
},

"message_of_day": {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Suggested change
"message_of_day": {
"message_of_the_day": {

Comment on lines 747 to 748
if scaleOfMode := raw["scale_down_mode"].(string); scaleOfMode != "" {
profile.ScaleDownMode = containerservice.ScaleDownMode(scaleOfMode)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if scaleOfMode := raw["scale_down_mode"].(string); scaleOfMode != "" {
profile.ScaleDownMode = containerservice.ScaleDownMode(scaleOfMode)
if scaleDownMode := raw["scale_down_mode"].(string); scaleDownMode != "" {
profile.ScaleDownMode = containerservice.ScaleDownMode(scaleDownMode)

@@ -336,6 +336,8 @@ A `default_node_pool` block supports the following:

* `max_pods` - (Optional) The maximum number of pods that can run on each agent. Changing this forces a new resource to be created.

* `message_of_day` - (Optional) A base64-encoded string which will be written to /etc/motd after decoding. This allows customization of the message of the day for Linux nodes. It must not be specified for Windows nodes. It must be a static string (i.e., will be printed raw and not be executed as a script). Changing this forces a new resource to be created.
Copy link
Member

Choose a reason for hiding this comment

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

If this can't be supplied for Windows nodes could we add a safeguard to prevent that in the create function?

Suggested change
* `message_of_day` - (Optional) A base64-encoded string which will be written to /etc/motd after decoding. This allows customization of the message of the day for Linux nodes. It must not be specified for Windows nodes. It must be a static string (i.e., will be printed raw and not be executed as a script). Changing this forces a new resource to be created.
* `message_of_the_day` - (Optional) A base64-encoded string which will be written to /etc/motd after decoding. This allows customization of the message of the day for Linux nodes. It cannot be specified for Windows nodes and must be a static string (i.e. will be printed raw and not executed as a script). Changing this forces a new resource to be created.

@@ -356,6 +358,8 @@ A `default_node_pool` block supports the following:

-> **Note:** This requires that the Preview Feature `Microsoft.ContainerService/PodSubnetPreview` is enabled and the Resource Provider is re-registered, see [the documentation](https://docs.microsoft.com/azure/aks/configure-azure-cni#register-the-podsubnetpreview-preview-feature) for more information.

* `scale_down_mode` - (Optional) This also effects the cluster autoscaler behavior. If not specified, it defaults to Delete. Possible values include: 'ScaleDownModeDelete', 'ScaleDownModeDeallocate'. Changing this forces a new resource to be created.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `scale_down_mode` - (Optional) This also effects the cluster autoscaler behavior. If not specified, it defaults to Delete. Possible values include: 'ScaleDownModeDelete', 'ScaleDownModeDeallocate'. Changing this forces a new resource to be created.
* `scale_down_mode` - (Optional) Specifies the autoscaling behaviour of the Kubernetes Cluster. If not specified, it defaults to 'ScaleDownModeDelete'. Possible values include 'ScaleDownModeDelete' and 'ScaleDownModeDeallocate'. Changing this forces a new resource to be created.

@@ -386,6 +390,8 @@ If `enable_auto_scaling` is set to `false`, then the following fields can also b

-> **Note:** If `enable_auto_scaling` is set to `false` both `min_count` and `max_count` fields need to be set to `null` or omitted from the configuration.

* `workload_runtime` - (Optional) Used to specify the workload runtime. Allowed values are `OCIContainer`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `workload_runtime` - (Optional) Used to specify the workload runtime. Allowed values are `OCIContainer`.
* `workload_runtime` - (Optional) Specifies the workload runtime used by the node pool. Possible values are `OCIContainer`.

@@ -548,6 +554,8 @@ A `load_balancer_profile` block supports the following:

* `managed_outbound_ip_count` - (Optional) Count of desired managed outbound IPs for the cluster load balancer. Must be between `1` and `100` inclusive.

* `managed_outbound_ipv6_count` - (Optional) The desired number of IPv6 outbound IPs created/managed by Azure for the cluster load balancer. Allowed values must be in the range of 1 to 100 (inclusive). The default value is 0 for single-stack and 1 for dual-stack.
Copy link
Member

Choose a reason for hiding this comment

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

If I recall dual-stack is still a preview feature that needs to be enabled in order for this to work - should we add a note below explaining this?

Suggested change
* `managed_outbound_ipv6_count` - (Optional) The desired number of IPv6 outbound IPs created/managed by Azure for the cluster load balancer. Allowed values must be in the range of 1 to 100 (inclusive). The default value is 0 for single-stack and 1 for dual-stack.
* `managed_outbound_ipv6_count` - (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.

@@ -97,6 +97,8 @@ The following arguments are supported:

* `max_pods` - (Optional) The maximum number of pods that can run on each agent. Changing this forces a new resource to be created.

* `message_of_day` - (Optional) A base64-encoded string which will be written to /etc/motd after decoding. This allows customization of the message of the day for Linux nodes. It must not be specified for Windows nodes. It must be a static string (i.e., will be printed raw and not be executed as a script). Changing this forces a new resource to be created.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `message_of_day` - (Optional) A base64-encoded string which will be written to /etc/motd after decoding. This allows customization of the message of the day for Linux nodes. It must not be specified for Windows nodes. It must be a static string (i.e., will be printed raw and not be executed as a script). Changing this forces a new resource to be created.
* `message_of_the_day` - (Optional) A base64-encoded string which will be written to /etc/motd after decoding. This allows customization of the message of the day for Linux nodes. It cannot be specified for Windows nodes and must be a static string (i.e. will be printed raw and not executed as a script). Changing this forces a new resource to be created.

@ms-henglu
Copy link
Contributor

Hi @stephybun , thank you for code review, I've updated the PR as suggested, please take another look, thanks!

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @qiqingzhang and @ms-henglu LGTM ✨

@stephybun stephybun merged commit fa66a4b into hashicorp:main Oct 6, 2022
@github-actions github-actions bot added this to the v3.26.0 milestone Oct 6, 2022
stephybun added a commit that referenced this pull request Oct 6, 2022
@github-actions
Copy link

This functionality has been released in v3.26.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants