New Resource: `azurerm_extended_location_custom_location` #24267

jiaweitao001 · 2023-12-18T09:00:23Z

All related tests passed.

internal/services/extendedlocation/extended_location_custom_location.go

ms-henglu · 2024-01-09T02:26:24Z

internal/services/extendedlocation/extended_location_custom_location.go

+		},
+
+		"authentication": {
+			Type:     pluginsdk.TypeList,


Could we improve the validation here?

Sure. Will contact service team's POC for requirements of this authentication.

Similarly here, do we have validation information / acceptable values for the docs from the Service Team?

value now has a validation func, service team did not provide any info about type.

internal/services/extendedlocation/extended_location_custom_location.go

ms-henglu · 2024-01-09T02:40:42Z

internal/services/extendedlocation/extended_location_custom_location_test.go

+  cluster_extension_ids = [
+    "${azurerm_arc_kubernetes_cluster_extension.test.id}"
+  ]
+  display_name     = "customlocation%[2]d"


I think we don't need to specify display_name in the basic config, because it's optional.

Will remove.

ms-henglu · 2024-01-09T02:41:22Z

internal/services/extendedlocation/extended_location_custom_location_test.go

+			),
+		},
+		data.ImportStep(),
+	})


Could we add one more step to test whether the resource could be able to update to the basic config?

This looks to have been missed?

It's not possible to update to basic from update in this case, because once display_name is set here, it cannot be removed.

ms-henglu · 2024-01-09T02:42:41Z

internal/services/extendedlocation/extended_location_custom_location_test.go

+		data.ImportStep(),
+	})
+}
+


Could we add one more testcase to test whether we could deploy the complete config directly instead of updating it from the basic config? And consider updating the config name from "update" to "complete"?

ms-henglu · 2024-01-09T02:54:06Z

internal/services/extendedlocation/extended_location_custom_location_test.go

+  resource_group_name = azurerm_resource_group.test.name
+  location            = azurerm_resource_group.test.location
+  cluster_extension_ids = [
+    "${azurerm_arc_kubernetes_cluster_extension.test.id}"


Could we remove the "${}", please also update other places.

ms-henglu · 2024-01-09T02:54:29Z

website/docs/r/extended_location_custom_location.html.markdown

+  resource_group_name = azurerm_resource_group.example.name
+  location            = "West Europe"
+  cluster_extension_ids = [
+    "${azurerm_arc_kubernetes_cluster_extension.test.id}"


Should it be "azurerm_arc_kubernetes_cluster_extension.example.id"?

ms-henglu

Thanks for this PR!
I have left some additional mostly minor comments but overall LGTM!

jackofallops

Thanks @jiaweitao001 - Just a couple more items (I think some code went missing in the rebase?) and I think this is good to go. Testing is looking good at this stage, so if you can check the comments I think we can get this merged.

jackofallops · 2024-08-05T06:53:25Z

internal/services/extendedlocation/extended_location_custom_location.go

+					state.Namespace = pointer.From(props.Namespace)
+				}
+
+				if props.Authentication != nil && props.Authentication.Type != nil && props.Authentication.Value != nil {


This feels like a bug, if the property is optional in the spec and all of it's properties are null, this should be null too? Can you open an issue on the spec to track and add a comment here?

jackofallops · 2024-08-05T06:55:30Z

internal/services/extendedlocation/extended_location_custom_location.go

+			d := metadata.ResourceData
+
+			if d.HasChanges("authentication") {
+				if len(state.Authentication) > 0 {


We seem to have lost this change to remove the auth when empty as part of the rebase?

jackofallops · 2024-08-05T07:20:03Z

internal/services/extendedlocation/extended_location_custom_location.go

+				Properties: pointer.To(customLocationProps),
+			}
+
+			if _, err := client.Update(ctx, *id, props); err != nil {


The number of properties doesn't really matter, it's the behaviour of the API and compatibility with Terraform's expectations. So, if we have one or more properties that are allowed to be removed entirely, such as authentication, we need to know how to unset those. If sending a nil for a value doesn't remove/unset it (as per the HTTP spec, this should be a no-op) we need to send the appropriate value to remove it.

To the point of Update vs CreateOrUpdate, the "location is required" error will not happen if we use a Get to retrieve the existing data, then simply update that with any user changes before sending it back, rather than using the Patch only method?

jackofallops

Thanks @jiaweitao001 - I think this looks good to go now, thanks for the changes and the persistence 👍

jackofallops · 2024-08-13T12:57:09Z

Tests passing:

outdated

github-actions · 2024-09-14T02:12:37Z

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 bot added documentation dependencies size/XL labels Dec 18, 2023

jiaweitao001 force-pushed the extended_location branch from e62b935 to 8fc0353 Compare December 18, 2023 09:05

github-actions bot added the waiting-response label Dec 18, 2023

jiaweitao001 force-pushed the extended_location branch from 8fc0353 to 52cf42d Compare December 19, 2023 02:38

github-actions bot added waiting-response and removed waiting-response labels Dec 19, 2023

jiaweitao001 force-pushed the extended_location branch from 52cf42d to 177625d Compare December 19, 2023 02:47

github-actions bot added waiting-response and removed waiting-response labels Dec 19, 2023

jiaweitao001 force-pushed the extended_location branch from 177625d to 22f3a0c Compare December 19, 2023 05:02

github-actions bot added waiting-response and removed waiting-response labels Dec 19, 2023

jiaweitao001 force-pushed the extended_location branch from 22f3a0c to 6dd7dab Compare December 19, 2023 06:08

github-actions bot added waiting-response and removed waiting-response labels Dec 19, 2023

jiaweitao001 force-pushed the extended_location branch from 6dd7dab to a8f490e Compare December 19, 2023 08:21

github-actions bot removed the waiting-response label Dec 19, 2023

jiaweitao001 force-pushed the extended_location branch from a8f490e to 35070dc Compare January 8, 2024 05:42

github-actions bot added waiting-response and removed waiting-response labels Jan 8, 2024

ms-henglu reviewed Jan 9, 2024

View reviewed changes

internal/services/extendedlocation/extended_location_custom_location.go Outdated Show resolved Hide resolved

github-actions bot added waiting-response and removed waiting-response labels Jan 9, 2024

ms-henglu suggested changes Jan 9, 2024

View reviewed changes

github-actions bot added the waiting-response label Jan 9, 2024

ms-henglu suggested changes Jan 9, 2024

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Jan 26, 2024

jiaweitao001 and others added 13 commits August 1, 2024 17:12

Update extension type

6fc7e35

update test ext

aeec103

New Resource: azurerm_extended_location_custom_location

7ef1c9d

fix conflict

3f0719d

Added validation func, more acc test, updated script and docs

3495b73

change name validation func

044c2be

remove location overrride in acc tests

a2b8eb3

Address comments, remove unused location overrides, add examples in doc

c1979cc

Fix indent in client list

5027d6c

address comments, change elem type, removing redundant code

c0876c8

Update vendors

ef9be02

update oid in python script, add more comments inline for users

51d1ef4

Address comments, remove some unupdatable attributes

b44259b

jiaweitao001 force-pushed the extended_location branch from 0f103d3 to b44259b Compare August 1, 2024 09:13

github-actions bot removed the waiting-response label Aug 1, 2024

revert

df7abf8

jackofallops requested changes Aug 5, 2024

View reviewed changes

github-actions bot added the waiting-response label Aug 5, 2024

address comments on update

4793fed

github-actions bot added waiting-response and removed waiting-response labels Aug 7, 2024

jiaweitao001 requested a review from jackofallops August 12, 2024 02:13

jackofallops approved these changes Aug 13, 2024

View reviewed changes

jackofallops merged commit 19ad4f1 into hashicorp:main Aug 13, 2024
37 checks passed

github-actions bot added this to the v3.116.0 milestone Aug 13, 2024

jackofallops added a commit that referenced this pull request Aug 13, 2024

Update CHANGELOG.md for #24267

aa166f5

github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_extended_location_custom_location` #24267

New Resource: `azurerm_extended_location_custom_location` #24267

jiaweitao001 commented Dec 18, 2023

ms-henglu Jan 9, 2024

jiaweitao001 Jan 9, 2024

jackofallops Jul 29, 2024

jiaweitao001 Jul 31, 2024

ms-henglu Jan 9, 2024

jiaweitao001 Jan 9, 2024

ms-henglu Jan 9, 2024

jiaweitao001 Jan 9, 2024

jackofallops Jul 24, 2024

jiaweitao001 Jul 26, 2024

ms-henglu Jan 9, 2024

jiaweitao001 Jan 9, 2024

ms-henglu Jan 9, 2024

jiaweitao001 Jan 9, 2024

ms-henglu Jan 9, 2024

ms-henglu left a comment

jackofallops left a comment

jackofallops Aug 5, 2024

jackofallops Aug 5, 2024

jackofallops Aug 5, 2024

jackofallops left a comment

jackofallops commented Aug 13, 2024

github-actions bot commented Sep 14, 2024

New Resource: azurerm_extended_location_custom_location #24267

New Resource: azurerm_extended_location_custom_location #24267

Conversation

jiaweitao001 commented Dec 18, 2023

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

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

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

ms-henglu left a comment

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

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops commented Aug 13, 2024

github-actions bot commented Sep 14, 2024

New Resource: `azurerm_extended_location_custom_location` #24267

New Resource: `azurerm_extended_location_custom_location` #24267