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.

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

Merged
merged 8 commits into from
May 3, 2024

Conversation

bhardwajRahul
Copy link
Member

This PR aims at adding support for Opensearch in Terraform Provider.

Copy link

gitguardian bot commented Apr 16, 2024

️✅ 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.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 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.

@bhardwajRahul bhardwajRahul marked this pull request as ready for review April 16, 2024 10:20
Copy link
Member

@andrewsomething andrewsomething left a 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?

@bhardwajRahul
Copy link
Member Author

bhardwajRahul commented Apr 16, 2024

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.

@andrewsomething
Copy link
Member

@bhardwajRahul Most of these changes do not seem to be coming from the godo update. On main:

asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ git checkout main
Already on 'main'
Your branch is up to date with 'origin/main'.
asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ go get github.com/digitalocean/godo@v1.113.0
go: upgraded github.com/digitalocean/godo v1.109.1-0.20240228180303-16a4709be517 => v1.113.0
asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ go mod vendor
asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ go mod tidy
asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ git st
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   go.mod
	modified:   go.sum
	modified:   vendor/github.com/digitalocean/godo/CHANGELOG.md
	modified:   vendor/github.com/digitalocean/godo/databases.go
	modified:   vendor/github.com/digitalocean/godo/godo.go
	modified:   vendor/github.com/digitalocean/godo/load_balancers.go
	modified:   vendor/modules.txt

no changes added to commit (use "git add" and/or "git commit -a")

@bhardwajRahul
Copy link
Member Author

@bhardwajRahul Most of these changes do not seem to be coming from the godo update. On main:

asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ git checkout main
Already on 'main'
Your branch is up to date with 'origin/main'.
asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ go get github.com/digitalocean/godo@v1.113.0
go: upgraded github.com/digitalocean/godo v1.109.1-0.20240228180303-16a4709be517 => v1.113.0
asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ go mod vendor
asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ go mod tidy
asb@asb-do-laptop:~/projects/terraform-provider-digitalocean$ git st
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   go.mod
	modified:   go.sum
	modified:   vendor/github.com/digitalocean/godo/CHANGELOG.md
	modified:   vendor/github.com/digitalocean/godo/databases.go
	modified:   vendor/github.com/digitalocean/godo/godo.go
	modified:   vendor/github.com/digitalocean/godo/load_balancers.go
	modified:   vendor/modules.txt

no changes added to commit (use "git add" and/or "git commit -a")

Thanks, Fixed..

@bhardwajRahul bhardwajRahul marked this pull request as draft April 16, 2024 16:34
Comment on lines 631 to 638
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)
}
Copy link
Member

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...

Copy link
Member Author

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.

@bhardwajRahul bhardwajRahul marked this pull request as ready for review April 18, 2024 15:20
@@ -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 {
Copy link
Contributor

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.

Copy link
Member Author

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.

@dweinshenker
Copy link
Contributor

What are your thoughts @andrewsomething and @bhardwajRahul wrt putting the ui connection properties into its own schema (i.e. ui_connection). This way we don't make the top-level properties too wide.

Copy link
Member

@andrewsomething andrewsomething left a 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.)

@andrewsomething andrewsomething merged commit 14d20e2 into digitalocean:main May 3, 2024
3 checks passed
@bhardwajRahul bhardwajRahul deleted the rbhardwaj/DBAAS-85 branch May 3, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants