new resource and data source : azurerm_oracle_cloud_vm_cluster #27176

eelhomsi · 2024-08-23T18:01:55Z

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

Introduce OracleDatabase VMCluster Resource & Data and DBServer Data
Most relevant changes in:
internal/services/oracledatabase/

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”

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)

VMCluster Resource tests (ran one by one due to tenancy resourcing constraints):

VM Cluster Data source:

DB Server Data source:

Change Log

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

FEATURES - Introduced new CloudVmClusterResource, CloudVmClusterDataSource, and DBServersDataSource`.

This is a (please select all that apply):

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

Note

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

katbyte

Thanks @eelhomsi - i've left some comments inline as well as triggered CI for thisPR

katbyte · 2024-09-05T23:03:26Z

internal/services/oracledatabase/client/client.go

+}
+
+func NewClient(o *common.ClientOptions) (*Client, error) {
+	o.DisableCorrelationRequestID = true


how come we are disabling this? please add a comment detailing why

Good point - I've added a comment explaining why we disabled this.

katbyte · 2024-09-05T23:04:16Z

internal/services/oracledatabase/cloud_vm_cluster_data_source.go

+		},
+		"cloud_exadata_infrastructure_id": {


there should be a new line here

Suggested change

},

"cloud_exadata_infrastructure_id": {

},

"cloud_exadata_infrastructure_id": {

Done, thanks!

katbyte · 2024-09-05T23:05:20Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+		"location": commonschema.Location(),
+		"name": {
+			Type:     pluginsdk.TypeString,
+			Required: true,
+		},
+		"resource_group_name": {


Suggested change

"location": commonschema.Location(),

"name": {

Type: pluginsdk.TypeString,

Required: true,

},

"resource_group_name": {

"location": commonschema.Location(),

"name": {

Type: pluginsdk.TypeString,

Required: true,

},

"resource_group_name": {

should be new lines here. apply to all other schema properties

I've added new lines for all the schema properties.

katbyte · 2024-09-05T23:05:28Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+			Type:     pluginsdk.TypeString,
+			Required: true,
+		},
+		"tags": commonschema.Tags(),


this should be at the bottom

internal/services/oracledatabase/cloud_vm_cluster_resource.go

katbyte · 2024-09-05T23:10:29Z

internal/services/oracledatabase/cloud_vm_cluster_resource_test.go

+  %s
+resource "azurerm_oracledatabase_cloud_vm_cluster" "test" {
+	location = "%[3]s"
+  	name = "OFakeVmacctest%[2]d"
+  	resource_group_name = azurerm_resource_group.test.name


this will need to be formatted with terrafmt

Done thanks!

internal/services/oracledatabase/cloud_vm_cluster_resource_test.go

katbyte · 2024-09-05T23:11:43Z

website/docs/r/oracledatabase_cloud_vm_cluster.html.markdown

+
+* `cpu_core_count` - (Required) The number of CPU cores enabled on the cloud VM cluster.
+
+* `db_servers` - (Required) The list of DB servers.


list of? hostnames/ ids? if host names

Suggested change

* `db_servers` - (Required) The list of DB servers.

* `db_server_host_namess` - (Required) The list of DB server hostnames to use.

@katbyte I agree with your comments on the naming of these arguments, however I have been asked to match the official OCI Terraform provider here (concerns about consistent customer experience), so I don't have a lot of freedom in changing them myself:
https://registry.terraform.io/providers/oracle/oci/latest/docs/data-sources/database_cloud_vm_cluster#db_servers

Typo db_server_host_namess

These changes were not used, so the typo was not committed.

katbyte · 2024-09-05T23:12:15Z

website/docs/r/oracledatabase_cloud_vm_cluster.html.markdown

+
+* `display_name` - (Required) The user-friendly name for the cloud VM cluster. The name does not need to be unique..
+
+* `gi_version` - (Required) A valid Oracle Grid Infrastructure (GI) software version.


we should avoid ambiguous acronyms

Suggested change

* `gi_version` - (Required) A valid Oracle Grid Infrastructure (GI) software version.

* `oracle_grid_nfrastructure_version` - (Required) A valid Oracle Grid Infrastructure (GI) software version.

Typo oracle_grid_nfrastructure_version.

These changes were not used, so the typo was not committed.

katbyte · 2024-09-05T23:12:57Z

website/docs/r/oracledatabase_cloud_vm_cluster.html.markdown

+
+* `cluster_name` - (Optional) The cluster name for cloud VM cluster.
+
+* `data_collection_options` - (Optional) A `data_collection_options` block as defined below.


Suggested change

* `data_collection_options` - (Optional) A `data_collection_options` block as defined below.

* `data_collection` - (Optional) A `data_collection_options` block as defined below.

…t.go Co-authored-by: kt <[email protected]>

…d-data-source' into introduce-vm-cluster-resource-and-data-source

katbyte · 2024-09-18T17:51:00Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+			Type:     pluginsdk.TypeString,
+			Required: true,
+		},
+		"resource_group_name": {


this still needs to be updated to use common schema

katbyte · 2024-09-18T17:51:10Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+			Type:     pluginsdk.TypeString,
+			Required: true,
+			ForceNew: true,
+		},


can we validate this is a proper id?

Done, I'm now calling the SDK provided validator.

katbyte · 2024-09-18T17:51:23Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+		"cpu_core_count": {
+			Type:     pluginsdk.TypeInt,
+			Required: true,
+		},


can we validate tyhis? i presume it cannot be -1 or 0?

katbyte · 2024-09-18T17:51:47Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+			Optional: true,
+			Computed: true,
+			ForceNew: true,
+		},


and here. please add reasonable validation to all properties where possible.

katbyte · 2024-09-18T17:52:09Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+			Type:             pluginsdk.TypeString,
+			Required:         true,
+			ForceNew:         true,
+			DiffSuppressFunc: GiVersionDiffSuppress,


why are we diff supressing here? please add a comment explaining

I don't think this is needed anymore, so I have removed it.

katbyte · 2024-09-18T17:58:03Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+			output.Hostname = result.Model.Properties.Hostname
+			output.LicenseModel = string(pointer.From(result.Model.Properties.LicenseModel))
+			output.MemorySizeInGbs = pointer.From(result.Model.Properties.MemorySizeInGbs)
+			//output.SshPublicKeys = result.Model.Properties.SshPublicKeys


why is this commented out?

Uncommented now!

katbyte · 2024-09-18T17:58:15Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+			output.VnetId = result.Model.Properties.VnetId
+			// Optional


Suggested change

output.VnetId = result.Model.Properties.VnetId

// Optional

output.VnetId = result.Model.Properties.VnetId

// Optional

Suggested change

output.VnetId = result.Model.Properties.VnetId

// Optional

output.VnetId = result.Model.Properties.VnetId

// Optional

katbyte · 2024-09-18T17:58:50Z

internal/services/oracledatabase/cloud_vm_cluster_resource_test.go

+  %s
+resource "azurerm_oracledatabase_cloud_vm_cluster" "test" {


Suggested change

%s

resource "azurerm_oracledatabase_cloud_vm_cluster" "test" {

%s

resource "azurerm_oracledatabase_cloud_vm_cluster" "test" {

Suggested change

%s

resource "azurerm_oracledatabase_cloud_vm_cluster" "test" {

%s

resource "azurerm_oracledatabase_cloud_vm_cluster" "test" {

katbyte · 2024-09-18T17:59:20Z

internal/services/oracledatabase/db_servers_data_source.go

+		"resource_group_name": {
+			Type:     pluginsdk.TypeString,
+			Required: true,
+		},


please use common schema here

katbyte · 2024-09-18T18:01:13Z

internal/services/oracledatabase/cloud_vm_cluster_resource.go

+		"name": {
+			Type:         pluginsdk.TypeString,
+			Required:     true,
+			ValidateFunc: validate.Name,


this is used for multiple different nameS? are they actually all constrained in the same way?

Yes both name and display_name will have the same constraints applied to them.

katbyte · 2024-09-18T18:06:48Z

internal/services/oracledatabase/cloud_vm_cluster_helpers.go

+	"strings"
+)
+
+func ConvertDataCollectionOptionsToInternal(dataCollectionOptions *cloudvmclusters.DataCollectionOptions) []DataCollectionOptionsModel {


These seem to be the expand/flatten functions? these should be in the same file they are used in & use the terminology the rest of the provider does

flatten functions take api response/data -> terraform/internal
expand functions take internal/tf -> api request

Thanks, done.

…e-vm-cluster-resource-and-data-source

katbyte

Just a comment comments left ineline

internal/services/oracle/cloud_vm_cluster_data_source.go

internal/services/oracle/cloud_vm_cluster_resource.go

katbyte · 2024-10-15T20:54:30Z

internal/services/oracle/cloud_vm_cluster_resource.go

+// Example: Initial plan -> testHostname, final result after DBaaS -> testHostname-abc.
+// Since testHostname is a prefix of testHostname-abc, then computed diff is zero.
+func DbSystemHostnameDiffSuppress(_ string, old string, new string, _ *schema.ResourceData) bool {
+	return EqualIgnoreCaseSuppressDiff(old, new) || NewIsPrefixOfOldDiffSuppress(old, new)


we shouldn't be ignoring case, terraform is case sensitive

instead of suppressing the diff we should/could strip the suffix on read

I've removed the ignoring case part. Can you clarify the second point?

internal/services/oracle/db_servers_data_source.go

website/docs/d/oracle_cloud_vm_cluster.html.markdown

internal/services/oracle/cloud_vm_cluster_resource.go

Co-authored-by: kt <[email protected]>

…e-vm-cluster-resource-and-data-source

eelhomsi · 2024-10-17T04:37:39Z

Test evidence here:

katbyte

LGTM 🏖️

#27176)" This reverts commit cb8d847.

jackofallops · 2024-10-17T08:27:20Z

Hi @eelhomsi - Apologies, but we've had to revert this PR. Since it's previously been merged, we cannot re-open from here. Can you please submit a new PR from the Branch and we'll work with you to get it merged?

Thanks!

eelhomsi · 2024-10-17T09:07:17Z

Hi @eelhomsi - Apologies, but we've had to revert this PR. Since it's previously been merged, we cannot re-open from here. Can you please submit a new PR from the Branch and we'll work with you to get it merged?

Thanks!

No problem @jackofallops PR recreated here:
#27677

github-actions · 2024-11-17T02:22:10Z

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.

eelhomsi added 4 commits August 9, 2024 21:44

* Add Exadata Infrastructure files.

90eb21b

Merge branch 'main' into introduce-exa-data-and-resource

9482eba

* Add docs for db_server data source.

22a45cc

* Update.

ac87fb3

github-actions bot added documentation dependencies size/XL labels Aug 23, 2024

* remove exa.

1436c16

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

eelhomsi added 2 commits September 3, 2024 16:32

* Format.

d26aada

* Update.

56107d7

github-actions bot removed the waiting-response label Sep 3, 2024

* Format.

c0f8837

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

katbyte requested changes Sep 5, 2024

View reviewed changes

eelhomsi added 3 commits September 9, 2024 08:51

* Move to bottom.

16a5e92

* Add validators.

61e90b7

* Explain disabling of correlationId being provided by AzureRM.

5500b39

github-actions bot removed the waiting-response label Sep 9, 2024

eelhomsi and others added 7 commits September 9, 2024 09:45

* Add new lines and move tags to bottom.

2bc1dc2

Update internal/services/oracledatabase/cloud_vm_cluster_resource_tes…

f4f3673

…t.go Co-authored-by: kt <[email protected]>

* Add validation for names.

d31df5a

Merge remote-tracking branch 'origin/introduce-vm-cluster-resource-an…

17ab2a2

…d-data-source' into introduce-vm-cluster-resource-and-data-source

* Format blocks.

26835d1

* Format blocks.

2cb6d46

* Format blocks.

6f35ff7

github-actions bot added the waiting-response label Sep 18, 2024

katbyte requested changes Sep 18, 2024

View reviewed changes

katbyte reviewed Sep 18, 2024

View reviewed changes

* Address comments.

19bc88c

eelhomsi added 3 commits October 15, 2024 00:52

* Add suppress comments.

99ae973

Merge remote-tracking branch 'refs/remotes/origin/main' into introduc…

8b46daa

…e-vm-cluster-resource-and-data-source

* Oracle Database -> Oracle.

05343c9

github-actions bot added the waiting-response label Oct 15, 2024

* Golint.

e392139

github-actions bot removed the waiting-response label Oct 15, 2024

* Fix.

4ff3e3d

katbyte requested changes Oct 15, 2024

View reviewed changes

eelhomsi and others added 8 commits October 15, 2024 22:31

Update website/docs/d/oracle_cloud_vm_cluster.html.markdown

86f256a

Co-authored-by: kt <[email protected]>

Update website/docs/d/oracle_cloud_vm_cluster.html.markdown

cac17f3

Co-authored-by: kt <[email protected]>

Merge remote-tracking branch 'refs/remotes/origin/main' into introduc…

db2fb8e

…e-vm-cluster-resource-and-data-source

* Fix.

f5fdc54

Merge remote-tracking branch 'refs/remotes/origin/main' into introduc…

0e23213

…e-vm-cluster-resource-and-data-source

* Remove unneeded function.

910ff5a

* Add hostname actual.

913d259

Merge remote-tracking branch 'refs/remotes/origin/main' into introduc…

45ec7f0

…e-vm-cluster-resource-and-data-source

* Update docs.

df29992

katbyte approved these changes Oct 17, 2024

View reviewed changes

katbyte changed the title ~~Introduce OracleDatabase VMCluster Resource & Data and DBServer Data~~ new resource and data source : azurerm_oracle_cloud_vm_cluster Oct 17, 2024

katbyte merged commit cb8d847 into hashicorp:main Oct 17, 2024
34 checks passed

github-actions bot added this to the v4.6.0 milestone Oct 17, 2024

katbyte added a commit that referenced this pull request Oct 17, 2024

Update CHANGELOG.md #27176

79cf6d0

jackofallops added a commit that referenced this pull request Oct 17, 2024

Revert "new resource and data source : azurerm_oracle_cloud_vm_cluster (

bd87b79

#27176)" This reverts commit cb8d847.

jackofallops mentioned this pull request Oct 17, 2024

Revert "new resource and data source : azurerm_oracle_cloud_vm_cluster" #27675

Merged

jackofallops added a commit that referenced this pull request Oct 17, 2024

Update CHANGELOG.md reverting #27176

a9e7137

github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2024


		* `cpu_core_count` - (Required) The number of CPU cores enabled on the cloud VM cluster.

		* `db_servers` - (Required) The list of DB servers.

	* `db_servers` - (Required) The list of DB servers.
	* `db_server_host_namess` - (Required) The list of DB server hostnames to use.


		* `display_name` - (Required) The user-friendly name for the cloud VM cluster. The name does not need to be unique..

		* `gi_version` - (Required) A valid Oracle Grid Infrastructure (GI) software version.

	* `gi_version` - (Required) A valid Oracle Grid Infrastructure (GI) software version.
	* `oracle_grid_nfrastructure_version` - (Required) A valid Oracle Grid Infrastructure (GI) software version.


		* `cluster_name` - (Optional) The cluster name for cloud VM cluster.

		* `data_collection_options` - (Optional) A `data_collection_options` block as defined below.

	* `data_collection_options` - (Optional) A `data_collection_options` block as defined below.
	* `data_collection` - (Optional) A `data_collection_options` block as defined below.

		%s
		resource "azurerm_oracledatabase_cloud_vm_cluster" "test" {

new resource and data source : azurerm_oracle_cloud_vm_cluster #27176

new resource and data source : azurerm_oracle_cloud_vm_cluster #27176

Conversation

eelhomsi commented Aug 23, 2024 • edited Loading

Community Note

Description

PR Checklist

Testing

Change Log

katbyte 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

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

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

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eelhomsi commented Oct 17, 2024

katbyte left a comment

Choose a reason for hiding this comment

jackofallops commented Oct 17, 2024

eelhomsi commented Oct 17, 2024 • edited Loading

github-actions bot commented Nov 17, 2024

eelhomsi commented Aug 23, 2024 •

edited

Loading

eelhomsi commented Oct 17, 2024 •

edited

Loading