-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Feature: enable Azure PostgreSQL Server testing #772
Conversation
@HadwaAbdelhalem PTAL |
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.
Thank you @omichels for your contribution. I left few comments on the PR/ also would you please execute the CI pipeline in your fork. #https://github.com/gruntwork-io/terratest/blob/master/.github/workflows/ci.yml.
|
||
This folder contains a Terraform module that deploys resources in [Azure](https://azure.microsoft.com/) to demonstrate how you can use Terratest to write automated tests for your Azure Terraform code. | ||
This module deploys a database for PostgreSQL. | ||
|
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.
Add a link to azure postgres database https://azure.microsoft.com/services/postgresql/ , not mysql
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.
resolved
1. Install [Terraform](https://www.terraform.io/) and make sure it's on your `PATH` | ||
1. Configure your Terratest [Go test environment](../README.md) | ||
1. `cd test/azure` | ||
1. `go build terraform_azure_mysqldb_example_test.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.
fix to build the terraform_azure_postgresqldb_example_test.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.
resolved
# DEPLOY A RESOURCE GROUP | ||
# --------------------------------------------------------------------------------------------------------------------- | ||
resource "azurerm_resource_group" "rg" { | ||
name = "postgresql-rg" |
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.
add location variable in variable.tf, then use it to set the location. see other azure samples for ref #
location = var.location |
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.
resolved
geo_redundant_backup_enabled = false | ||
auto_grow_enabled = true | ||
|
||
administrator_login = "pgsqladmin" |
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.
please use random_password to create the administrator_login_password, See sample #
# Random password is used as an example to simplify the deployment and improve the security of the database. |
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.
resolved
} | ||
|
||
output "servername" { | ||
value = azurerm_postgresql_server.postgresqlserver.name |
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.
extra newline
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.
resolved
|
||
output "rgname" { | ||
value = azurerm_resource_group.rg.name | ||
} |
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.
add newline to the end of the file, consider running terraform fmt
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.
used terraform fmt
|
||
// website::tag::4:: Get the Server details and assert them against the terraform output | ||
actualServer := azure.GetPostgresqlServer(t, rgName, actualServername, subscriptionID) | ||
// Verify |
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.
please assert the actualServer is not nil instead
|
||
} | ||
|
||
output "rgname" { |
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.
NIT: Please rename to "resource_group_name" and move to the top of the file
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.
resolved
Hello. This PR is very useful for us so I would like to ask, is there any chance that this functionality will be accepted? |
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.
Thanks @omichels . @Jfmask yes, it is in final review phase . @yorinasub17 this is ready for your review
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! Had one more update on function consistency, but should be able to merge once that is addressed!
modules/azure/postgresql.go
Outdated
|
||
// GetPostgresqlServer is a helper function that gets the server. | ||
// This function would fail the test if there is an error. | ||
func GetPostgresqlServer(t testing.TestingT, resGroupName string, serverName string, subscriptionID string) *postgresql.Server { |
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 you make all the functions consistent and use PostgreSQL
instead of Postgresql
?
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.
@omichels could you please update the PR to resolve the last comment, or give me access to your repo and I would do it.
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.
"all functions consistent using PostgreSQL"
commit: 00727ae
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.
Tested and approved, @yorinasub17 ready for your final check.
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! Thanks for the contribution! Will merge and release now.
No description provided.