-
Notifications
You must be signed in to change notification settings - Fork 135
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
Prometheus for database #787
base: master
Are you sure you want to change the base?
Conversation
Waiting some backend devs on this feature to test with last version of the feature before merge this PR |
Backend development finished and resources tested with |
ovh/types_cloud_project_database.go
Outdated
@@ -85,6 +85,7 @@ type CloudProjectDatabaseResponse struct { | |||
Version string `json:"version"` | |||
Disk CloudProjectDatabaseDisk `json:"disk"` | |||
AdvancedConfiguration map[string]string `json:"advancedConfiguration"` | |||
EnablePrometheus bool `json:"EnablePrometheus"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnablePrometheus bool `json:"EnablePrometheus"` | |
EnablePrometheus bool `json:"enablePrometheus"` |
ovh/types_cloud_project_database.go
Outdated
) | ||
|
||
res := &CloudProjectDatabasePrometheusAccessResponse{} | ||
//log.Printf("[DEBUG] Will create pro: %+v for cluster %s from project %s", params, clusterId, serviceName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//log.Printf("[DEBUG] Will create pro: %+v for cluster %s from project %s", params, clusterId, serviceName) |
Required: true, | ||
}, | ||
"password_reset": { | ||
Type: schema.TypeString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really prefer a bool
property here as it will be way clearer for the end user. I understand that it requires him to input true
on a first apply and then set the field to false
to avoid re-resetting his password, but I think it's better this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not how terraform is use, we cannot ask our user to change the resource between the first and second apply. The user must be able to aply 5 time his code without update if he do not change the code.
We use the same strategy for user resources, and for all theses resources the documentation explain how to use it.
Our customers already use it without issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about password_reset_last_request_date
with datetime
format ?
That would be clearer IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @amstuta that string
is clearly not the way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Force datetime format is possible but in fact it's just a string with validate function (https://developer.hashicorp.com/terraform/plugin/sdkv2/schemas/schema-types#date-time-data).
It's just a trigger attribute with this documentation :
password_reset
- (Optional) Arbitrary string to change to trigger a password update. Use theterraform refresh
command after executingterraform apply
to update the output with the new password.
From my point of view, add a validate function will just add complexity for the user.
Also, if I change it here I have to change it in user resources and that will be a kind of breaking change.
ovh/types_cloud_project_database.go
Outdated
} | ||
d.SetId("") | ||
return nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
# ovh_cloud_project_database_prometheus (Data Source) | ||
|
||
Use this data source to get information about a prometheus of a database cluster associated with a public cloud project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional, but maybe you could add something like for mongodb, please use the other datasource ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change type datetime
Description
Add resource to manage Prometheus for Databases Services
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
make testacc TESTARGS="-run TestAccCloudProjectDatabaseMongodbPrometheusDataSource_basic"
make testacc TESTARGS="-run TestAccCloudProjectDatabasePrometheusDataSource_basic"
make testacc TESTARGS="-run TestAccCloudProjectDatabasePrometheus_importBasic"
make testacc TESTARGS="-run TestAccCloudProjectDatabaseMongodbPrometheus_basic"
make testacc TESTARGS="-run TestAccCloudProjectDatabasePrometheus_basic"
Test Configuration: