-
Notifications
You must be signed in to change notification settings - Fork 282
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
Terraform Provider updates for Opensearch #1140
Terraform Provider updates for Opensearch #1140
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
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.
Can we limit the dependency updates to what is strictly required for the change?
I think here we are doing a major godo version upgrade i.e. from 109 -> 113 and it is bringing along a lot of vendor/other dependencies with it. |
@bhardwajRahul Most of these changes do not seem to be coming from the godo update. On main:
|
9c35402
to
9f19cf6
Compare
Thanks, Fixed.. |
if database.UIConnection != nil { | ||
d.Set("host", database.UIConnection.Host) | ||
d.Set("port", database.UIConnection.Port) | ||
d.Set("uri", database.UIConnection.URI) | ||
d.Set("database", database.UIConnection.Database) | ||
d.Set("user", database.UIConnection.User) | ||
d.Set("password", database.UIConnection.Password) | ||
} |
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.
This overwrites the normal connection info when database.UIConnection
exists. Is that the intention? If not, we should extend the schema to include new attributes like ui_host
, ui_port
, etc...
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.
Previously, I thought we might not require connection details and only Dashboard connection details are required in case of opensearch but now I have updated the code and added both.
a9c246c
to
0abe54e
Compare
@@ -524,6 +557,13 @@ func resourceDigitalOceanDatabaseClusterRead(ctx context.Context, d *schema.Reso | |||
if err != nil { | |||
return diag.Errorf("Error setting connection info for database cluster: %s", err) | |||
} | |||
|
|||
if database.EngineSlug == opensearchEngineSlug { |
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.
not sure we need to check opensearch specifically, we have a check in the setUIConnectionInfo
for database.UIConnection != nil
. Just so it can be general purpose for other ui connection details for other engine types.
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 guess, if we do have plans of incorporating ui connection into other engines also then ya it make sense to skip this check for opensearch.
What are your thoughts @andrewsomething and @bhardwajRahul wrt putting the ui connection properties into its own schema (i.e. |
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.
👍 LGTM!
(Note I did push an additional commit with some changes to the documentation.)
This PR aims at adding support for Opensearch in Terraform Provider.