`azurerm_kubernetes_cluster` - support for `upgrade_override_setting` property #27962

hqhqhqhqhqhqhqhqhqhqhq · 2024-11-11T01:54:58Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Support for upgrade_override_setting property in azurerm_kubernetes_cluster.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_resource - support for the thing1 property [GH-00000]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

feat: implement upgrade settings for container service Revert "feat: implement upgrade settings for container service"

wuxu92 · 2024-11-22T08:20:28Z

internal/services/containers/kubernetes_cluster_resource.go

+				MaxItems: 1,
+				Elem: &pluginsdk.Resource{
+					Schema: map[string]*pluginsdk.Schema{
+						"effective_until": {


According to the guideline suggestions, we may make it a top level property.

terraform-provider-azurerm/contributing/topics/schema-design-considerations.md

Lines 75 to 85 in 778c935

Finally there are instances where the addtional fields/properties for a object/feature are optional or few in number, as shown below.

Example C.

```go

type ManagedClusterStorageProfileDiskCSIDriver struct {

Enabled *bool `json:"enabled,omitempty"`

Version *string `json:"version,omitempty"`

}

```

In cases like these one option is to flatten the block into two top level properties:

@wuxu92, totally agree with you here, however it appears that not all of the possible fields have been exposed here as well based on the REST API documentation for this property. We should also drop the _settings part from the name of the exposed field in the provider, also per the contributing guide. 🙂

For blocks avoid redundant words in the name that don't add informational value e.g.

firewall_properties can be shortened to firewall, the same can apply to individual properties e.g. email_address to email.

The only other field in the block is forceUpgrade as a boolean, which doesn't need to be exposed. So we may only need to expose one property named upgrade_override_until and keep the expand/flatten logic as it is now.

I am hesistant to add this as a top level as service team indicated there may be additional properties relevant to upgrade_override_setting added later. We may, in future, need to wrap around these properties a layer down in upgrade_override_setting as it doesn't make sense to expose all of them in top level.

I will update the naming to upgrade_override.

@wuxu92 @WodansSon

jackofallops

Thanks @hqhqhqhqhqhqhqhqhqhqhq - this looks mostly good, but I have questions on the schema design and the testing approach if you can take a look?

jackofallops · 2024-11-22T09:24:05Z

internal/services/containers/kubernetes_cluster_resource.go

+			"upgrade_override_setting": {
+				Type:     pluginsdk.TypeList,
+				Optional: true,
+				MaxItems: 1,
+				Elem: &pluginsdk.Resource{
+					Schema: map[string]*pluginsdk.Schema{
+						"effective_until": {
+							Type:         pluginsdk.TypeString,
+							Optional:     true,
+							ValidateFunc: validation.IsRFC3339Time,
+						},
+					},
+				},
+			},


Is this the only configurable property in this block? If so, it might be worth flattening this down to a single property, for example upgrade_override_until?

What happens to this value when the specified point in time passes, is it removed from the response, or still returned?

Should we also validate that the supplied time is in the future?

Hi @jackofallops

There is only one property that user should configure, but service team indicated more may be added later.

When the value is in the past in time, it will stay in the response

I prefer not to add this check because if the time is in the past, users don't need to update it every single time; they can leave it as is (and it will not have any impact).

jackofallops · 2024-11-22T09:27:36Z

internal/services/containers/kubernetes_cluster_resource_test.go

+	if isUpgradeOverrideSettingEnabled {
+		upgradeOverrideSetting = `
+  upgrade_override_setting {
+	effective_until = "2024-01-01T00:00:00Z"


Does it make sense to be testing a time in the past here? It's unlikely this will be representative of user input? Could we generate a value for this based on time.Now() plus some realistic duration?

hqhqhqhqhqhqhqhqhqhqhq · 2024-12-02T11:19:38Z

Hi @jackofallops @WodansSon
Thank you for review, I have made updates according to requested and changes and ready for re-review.
Please take another look when available and let me know anythings to change, thanks!

Also reran the test:

WodansSon · 2024-12-04T20:13:06Z

@hqhqhqhqhqhqhqhqhqhqhq, Thanks for pushing those changes, this LGTM 🚀

andyliuliming · 2024-12-16T02:54:46Z

when will this merge?

hqhqhqhqhqhqhqhqhqhqhq · 2024-12-16T03:04:08Z

Hi @andyliuliming , Hashicorp team scheduled a final official review on 19th of December. If everything is good, it will be merged & released soon after.

…

________________________________ From: Liming Liu ***@***.***> Sent: Monday, December 16, 2024 1:55:09 PM To: hashicorp/terraform-provider-azurerm ***@***.***> Cc: Harry Qu ***@***.***>; Mention ***@***.***> Subject: Re: [hashicorp/terraform-provider-azurerm] `azurerm_kubernetes_cluster` - support for `upgrade_override_setting` property (PR #27962) when will this merge? — Reply to this email directly, view it on GitHub<#27962 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BJC4MBYVM6XQAMCBYDD36XL2FY6I3AVCNFSM6AAAAABRQ2PIMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBUGQZTINJWGU>. You are receiving this because you were mentioned.Message ID: ***@***.***>

WodansSon

@hqhqhqhqhqhqhqhqhqhqhq , thanks for pushing those changes. This looks pretty good, one last suggestion in the documentation, after that's fix I think this PR will be good to go! 🚀

website/docs/r/kubernetes_cluster.html.markdown

Co-authored-by: Wodans Son <[email protected]>

WodansSon

So sorry, I missed this one in the last review. 😞

website/docs/r/kubernetes_cluster.html.markdown

Co-authored-by: Wodans Son <[email protected]>

WodansSon

Thanks for pushing those changes @hqhqhqhqhqhqhqhqhqhqhq . This LGTM now! 🚀

WodansSon · 2025-01-22T10:00:04Z

All of the tests are failing, if you can fix those up that would be great!

…hub.com/hqhqhqhqhqhqhqhqhqhqhq/terraform-provider-azurerm into feat/container_service-upgrade-settings

WodansSon · 2025-01-24T03:47:27Z

Some tests are still failing, but they are also failing in main:

WodansSon

@hqhqhqhqhqhqhqhqhqhqhq , this is getting very close, however I have left a couple of new comments I would like to see addressed. Once that is done, and the tests pass. I think this should be good to go and get merged! 🚀

WodansSon · 2025-01-29T07:18:40Z

internal/services/containers/kubernetes_cluster_resource.go

+	if d.HasChange("upgrade_override") {
+		upgradeOverrideSettingRaw := d.Get("upgrade_override").([]interface{})
+
+		err = validateKubernetesClusterUpgradeOverrideSetting(d.GetChange("upgrade_override"))


Would it make more sense to put this logic inline rather than in a helper function to be more consistent with the other resources in the provider? I might be wrong but I believe this is the only place this function is called, so to me it would make more sense to make that logic inline, here in the update fuction, to be more readable for future maintainers is they need to modify this code.

internal/services/containers/kubernetes_cluster_resource.go

WodansSon · 2025-01-29T07:23:20Z

internal/services/containers/kubernetes_cluster_resource.go

+
+	newOverrideSetting, canConvert := new.([]interface{})
+	if !canConvert || len(newOverrideSetting) == 0 {
+		return fmt.Errorf("upgrade_override cannot be unset")


Same here...

Suggested change

return fmt.Errorf("upgrade_override cannot be unset")

return fmt.Errorf("`upgrade_override` cannot be unset")

WodansSon · 2025-01-29T07:26:31Z

internal/services/containers/kubernetes_cluster_resource.go

@@ -2531,6 +2568,19 @@ func resourceKubernetesClusterUpdate(d *pluginsdk.ResourceData, meta interface{}
 	return resourceKubernetesClusterRead(d, meta)
 }

+func validateKubernetesClusterUpgradeOverrideSetting(_, new interface{}) error {


Can we remove this function and make it inline in the update function? I think we should be able to use something like d.GetRawConfig so we could compare the old values of the fields against the new values of the fields.

Updated to inline function, but left it as d.Get("upgrade_override") rather than d.GetRawConfig because it is easier to work with the expand function below (easier to parse to []interface{}).

WodansSon

One last thing, it appears the annotation for Notes has a new contributor docs guideline page dedicated to it so I added one more comment to address this in this PR as well. 🙂

website/docs/r/kubernetes_cluster.html.markdown

Co-authored-by: Wodans Son <[email protected]>

…hub.com/hqhqhqhqhqhqhqhqhqhqhq/terraform-provider-azurerm into feat/container_service-upgrade-settings

Update kubernetes_cluster.html.markdown

6134a08

feat: implement upgrade settings for container service Revert "feat: implement upgrade settings for container service"

github-actions bot added documentation size/M waiting-response labels Nov 11, 2024

Update kubernetes_cluster_resource.go

d17f687

github-actions bot added waiting-response and removed waiting-response labels Nov 11, 2024

Update kubernetes_cluster_resource.go

b8ff724

github-actions bot removed the waiting-response label Nov 11, 2024

update test case to include update operations for upgradeOverrideSetting

e341b33

hqhqhqhqhqhqhqhqhqhqhq marked this pull request as ready for review November 11, 2024 05:16

hqhqhqhqhqhqhqhqhqhqhq requested review from katbyte and a team as code owners November 11, 2024 05:16

rcskosir added enhancement service/kubernetes-cluster labels Nov 11, 2024

wuxu92 reviewed Nov 22, 2024

View reviewed changes

github-actions bot added the waiting-response label Nov 22, 2024

jackofallops requested changes Nov 22, 2024

View reviewed changes

fix: upgrade upgrade_override_setting to upgrade_override

ac5136a

github-actions bot added waiting-response and removed waiting-response labels Nov 22, 2024

Update kubernetes_cluster_resource_test.go

21c5107

github-actions bot added waiting-response and removed waiting-response labels Dec 2, 2024

update

5514082

github-actions bot removed the waiting-response label Dec 2, 2024

Update kubernetes_cluster_resource.go

0e21eff

github-actions bot added the waiting-response label Jan 20, 2025

Update kubernetes_cluster_resource.go

01a3af9

github-actions bot removed the waiting-response label Jan 20, 2025

update

f175ce7

github-actions bot added size/L and removed size/M labels Jan 20, 2025

WodansSon requested changes Jan 22, 2025

View reviewed changes

website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved

github-actions bot added the waiting-response label Jan 22, 2025

Update website/docs/r/kubernetes_cluster.html.markdown

07b3b1e

Co-authored-by: Wodans Son <[email protected]>

github-actions bot removed the waiting-response label Jan 22, 2025

WodansSon reviewed Jan 22, 2025

View reviewed changes

website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved

github-actions bot added the waiting-response label Jan 22, 2025

Update website/docs/r/kubernetes_cluster.html.markdown

d28c5e3

Co-authored-by: Wodans Son <[email protected]>

github-actions bot removed the waiting-response label Jan 22, 2025

WodansSon previously approved these changes Jan 22, 2025

View reviewed changes

hqhqhqhqhqhqhqhqhqhqhq added 2 commits January 23, 2025 01:04

Update kubernetes_cluster_resource.go

a3dcc8c

Merge branch 'feat/container_service-upgrade-settings' of https://git…

a0f05dc

…hub.com/hqhqhqhqhqhqhqhqhqhqhq/terraform-provider-azurerm into feat/container_service-upgrade-settings

hqhqhqhqhqhqhqhqhqhqhq dismissed WodansSon’s stale review via a0f05dc January 22, 2025 12:04

Update kubernetes_cluster_resource.go

aecf203

WodansSon requested changes Jan 29, 2025

View reviewed changes

github-actions bot added the waiting-response label Jan 29, 2025

WodansSon requested changes Jan 29, 2025

View reviewed changes

website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved

Update website/docs/r/kubernetes_cluster.html.markdown

f3cf8bd

Co-authored-by: Wodans Son <[email protected]>

github-actions bot removed the waiting-response label Jan 29, 2025

hqhqhqhqhqhqhqhqhqhqhq and others added 3 commits January 30, 2025 11:28

Update internal/services/containers/kubernetes_cluster_resource.go

a33befb

Co-authored-by: Wodans Son <[email protected]>

Update kubernetes_cluster_resource.go

5ac6fdd

Merge branch 'feat/container_service-upgrade-settings' of https://git…

1eb8891

…hub.com/hqhqhqhqhqhqhqhqhqhqhq/terraform-provider-azurerm into feat/container_service-upgrade-settings

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_kubernetes_cluster` - support for `upgrade_override_setting` property #27962

`azurerm_kubernetes_cluster` - support for `upgrade_override_setting` property #27962

hqhqhqhqhqhqhqhqhqhqhq commented Nov 11, 2024

wuxu92 Nov 22, 2024

WodansSon Nov 22, 2024 •

edited

Loading

wuxu92 Nov 22, 2024

hqhqhqhqhqhqhqhqhqhqhq Nov 22, 2024 •

edited

Loading

jackofallops left a comment

jackofallops Nov 22, 2024

hqhqhqhqhqhqhqhqhqhqhq Dec 2, 2024

jackofallops Nov 22, 2024

hqhqhqhqhqhqhqhqhqhqhq Dec 2, 2024

hqhqhqhqhqhqhqhqhqhqhq commented Dec 2, 2024

WodansSon commented Dec 4, 2024

andyliuliming commented Dec 16, 2024

hqhqhqhqhqhqhqhqhqhqhq commented Dec 16, 2024 via email •

edited

Loading

WodansSon left a comment

WodansSon left a comment •

edited

Loading

WodansSon left a comment

WodansSon commented Jan 22, 2025

WodansSon commented Jan 24, 2025

WodansSon left a comment

WodansSon Jan 29, 2025

WodansSon Jan 29, 2025

WodansSon Jan 29, 2025

hqhqhqhqhqhqhqhqhqhqhq Jan 29, 2025

WodansSon left a comment

	Finally there are instances where the addtional fields/properties for a object/feature are optional or few in number, as shown below.

	Example C.
	```go
	type ManagedClusterStorageProfileDiskCSIDriver struct {
	Enabled *bool `json:"enabled,omitempty"`
	Version *string `json:"version,omitempty"`
	}
	```

	In cases like these one option is to flatten the block into two top level properties:

	return fmt.Errorf("upgrade_override cannot be unset")
	return fmt.Errorf("`upgrade_override` cannot be unset")

azurerm_kubernetes_cluster - support for upgrade_override_setting property #27962

Are you sure you want to change the base?

azurerm_kubernetes_cluster - support for upgrade_override_setting property #27962

Conversation

hqhqhqhqhqhqhqhqhqhqhq commented Nov 11, 2024

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Choose a reason for hiding this comment

WodansSon Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hqhqhqhqhqhqhqhqhqhqhq Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hqhqhqhqhqhqhqhqhqhqhq commented Dec 2, 2024

WodansSon commented Dec 4, 2024

andyliuliming commented Dec 16, 2024

hqhqhqhqhqhqhqhqhqhqhq commented Dec 16, 2024 via email • edited Loading

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon left a comment • edited Loading

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon commented Jan 22, 2025

WodansSon commented Jan 24, 2025

WodansSon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

`azurerm_kubernetes_cluster` - support for `upgrade_override_setting` property #27962

`azurerm_kubernetes_cluster` - support for `upgrade_override_setting` property #27962

WodansSon Nov 22, 2024 •

edited

Loading

hqhqhqhqhqhqhqhqhqhqhq Nov 22, 2024 •

edited

Loading

hqhqhqhqhqhqhqhqhqhqhq commented Dec 16, 2024 via email •

edited

Loading

WodansSon left a comment •

edited

Loading