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.

Sign up for GitHub

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

Jump to bottom

Add disk in database resource creation #333

Merged
merged 5 commits into from
Nov 8, 2022
Merged

Add disk in database resource creation #333

merged 5 commits into from
Nov 8, 2022

Conversation

Galiley
Copy link
Contributor

@Galiley Galiley commented Nov 2, 2022

No description provided.

lpatte
lpatte requested changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@lpatte lpatte left a comment

Choose a reason for hiding this comment

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

Need To add it In

  • type CloudProjectDatabaseUpdateOpts struct and func (opts *CloudProjectDatabaseUpdateOpts) FromResource() to allow disksize to be updated
  • type CloudProjectDatabaseResponse struct and func (v CloudProjectDatabaseResponse) ToMap() to fill correctly the terraform state and avoid eternal update
  • ovh/data_cloud_project_database.go to get disksize in the datasource

ovh/resource_cloud_project_database.go
Type: schema.TypeInt,
Description: "Disk size attributes of the cluster",
Optional: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use ValidateFunc to check here that disk_size is > 0.
Like in ovh/resource_cloud_project_database_kafka_topic.go

ovh/types_cloud_project_database.go Outdated
Plan string `json:"plan"`
SubnetId string `json:"subnetId,omitempty"`
Version string `json:"version"`
}

type CloudProjectDatabaseDisk struct {
Size int `json:"size"`
Copy link
Contributor

Choose a reason for hiding this comment

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

omitempty

ovh/types_cloud_project_database.go Outdated
return fmt.Errorf("the disk size needs to be positive"), nil
}
opts.Disk = CloudProjectDatabaseDisk{diskSize}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need with both previous validateFunc and the omitempty.
You can just

opts.Disk = CloudProjectDatabaseDisk{
  Size: d.Get("disk_size").(int)
}

like CloudProjectDatabaseM3dbNamespaceCreateOpts Retention field

@Galiley Galiley force-pushed the set_disk_size_on_cluster_creation branch from a1a7147 to 7ceaa57 Compare November 3, 2022 13:28
lpatte
lpatte requested changes Nov 3, 2022
View reviewed changes
ovh/types_cloud_project_database.go Outdated
@@ -71,6 +73,7 @@ func (v CloudProjectDatabaseResponse) ToMap() map[string]interface{} {
obj["plan"] = v.Plan
obj["status"] = v.Status
obj["version"] = v.Version
obj["disk"] = v.Disk.ToMap()
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be align with schema because that will be set in terraform state an compare with the schema:

obj["disk_size"] = v.Disk.Size
obj["disk_type"] = v.Disk.Type // This need to add disk_type as computed attribut in the schema

ovh/types_cloud_project_database.go
}

func validateCloudProjectDatabaseDiskSize(v interface{}, k string) (ws []string, errors []error) {
errors = validateIsSupEqual(v.(int), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

only sup ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first the goal of this check was to avoid making request to the OVH API with negative values that would failed every time. And thus, adding more check will be redundant.

ovh/types_cloud_project_database.go Outdated
Plan string `json:"plan"`
SubnetId string `json:"subnetId,omitempty"`
Version string `json:"version"`
}

type CloudProjectDatabaseDisk struct {
Type string `json:"type,omitempty"`
Size *int `json:"size,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

really need pointer ? without it we can remove GetNilIntPointer()
d.Get("disk_size")will give 0 when not set and that will not be push thanks to omitempty

website/docs/r/cloud_project_database.html.markdown
@@ -243,6 +245,7 @@ The following attributes are exported:
* `plan` - See Argument Reference above.
* `status` - Current status of the cluster.
* `version` - See Argument Reference above.
* `disk_size` - See Argument Reference above.
Copy link
Contributor

Choose a reason for hiding this comment

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

if you add disk_type as computed, need it here

@Galiley Galiley changed the title Add disk size in database resource creation Add disk in database resource creation Nov 3, 2022
@Galiley Galiley force-pushed the set_disk_size_on_cluster_creation branch 2 times, most recently from 5aeb3aa to 69c7cf3 Compare November 3, 2022 16:37
@Galiley Galiley force-pushed the set_disk_size_on_cluster_creation branch from 69c7cf3 to 711d1e7 Compare November 3, 2022 16:40
@Galiley Galiley force-pushed the set_disk_size_on_cluster_creation branch from 3d16aa9 to 196d291 Compare November 8, 2022 12:49
scraly
scraly requested changes Nov 8, 2022
View reviewed changes
website/docs/r/cloud_project_database.html.markdown Outdated
@@ -207,6 +207,8 @@ The following arguments are supported:

* `opensearch_acls_enabled` - (Optional) Defines whether the ACLs are enabled on an OpenSearch cluster

* `disk_size` - (Optional) Defines the disk size of the database service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `disk_size` - (Optional) Defines the disk size of the database service.
* `disk_size` - (Optional) The disk size of the database service.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to know the unit too?

    disk_size               = 80

As a user I don't know "80" what? :-)

website/docs/d/cloud_project_database.html.markdown Outdated
@@ -68,3 +68,5 @@ The following attributes are exported:
* `plan` - Plan of the cluster.
* `status` - Current status of the cluster.
* `version` - The version of the engine in which the service should be deployed
* `disk_size` - Defines the disk size of the database service.
* `disk_type` - Defines the disk type of the database service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `disk_type` - Defines the disk type of the database service.
* `disk_type` - The disk type of the database service.

website/docs/r/cloud_project_database.html.markdown
@@ -243,6 +245,8 @@ The following attributes are exported:
* `plan` - See Argument Reference above.
* `status` - Current status of the cluster.
* `version` - See Argument Reference above.
* `disk_size` - See Argument Reference above.
* `disk_type` - Defines the disk type of the database service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `disk_type` - Defines the disk type of the database service.
* `disk_type` - See Argument Reference above.

@scraly scraly added the 0.23.0 label Nov 8, 2022
@scraly
Copy link
Collaborator

scraly commented Nov 8, 2022

Thanks :)

@scraly scraly merged commit 1773cc8 into ovh:master Nov 8, 2022
@Galiley Galiley deleted the set_disk_size_on_cluster_creation branch November 8, 2022 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scraly scraly scraly requested changes

@lpatte lpatte lpatte requested changes

Assignees
No one assigned
Labels
0.23.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Galiley @scraly @lpatte