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

New Resource - azurerm_postgresql_flexible_server_database (#11538) #12550

Merged
merged 2 commits into from
Aug 5, 2021

Conversation

wasfree
Copy link
Contributor

@wasfree wasfree commented Jul 12, 2021

New Resource
azurerm_postgresql_flexible_server_database

Noteworthy issues (Both issues already addressed to MS Product Group):

  • Database created via API/Cli owned by azuresu instead of azure_pg_admin
  • Database collation en_US is not accepted parameter, we have to use en_US.utf8

This PR address
#11538

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @wasfree - this looks good except for a failing tests:

------- Stdout: -------
=== RUN   TestAccPostgresqlFlexibleServerDatabase_requiresImport
=== PAUSE TestAccPostgresqlFlexibleServerDatabase_requiresImport
=== CONT  TestAccPostgresqlFlexibleServerDatabase_requiresImport
    testcase.go:89: Step 2/2, expected an error with pattern, no match on: Error running pre-apply refresh: exit status 1
        
        Error: Duplicate resource "azurerm_postgresql_flexible_server_database" configuration
        
          on terraform_plugin_test.tf line 35:
          35: resource "azurerm_postgresql_flexible_server_database" "test" {
        
        A azurerm_postgresql_flexible_server_database resource named "test" was
        already declared at terraform_plugin_test.tf:27,1-62. Resource names must be
        unique per type in each module.
    testing_new.go:63: Error retrieving state, there may be dangling resources: exit status 1
        
        Error: Duplicate resource "azurerm_postgresql_flexible_server_database" configuration
        
          on terraform_plugin_test.tf line 35:
          35: resource "azurerm_postgresql_flexible_server_database" "test" {
        
        A azurerm_postgresql_flexible_server_database resource named "test" was
        already declared at terraform_plugin_test.tf:27,1-62. Resource names must be
        unique per type in each module.
        
--- FAIL: TestAccPostgresqlFlexibleServerDatabase_requiresImport (377.32s)
FAIL

Copy link
Contributor

@yupwei68 yupwei68 left a comment

Choose a reason for hiding this comment

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

Hi, overall this looks good to me. I'm doing the same work currently. As I raise the problem of these fields are optional, the service team is fixing this problem.
Thanks!

@wasfree wasfree force-pushed the f/postgresql-flex-database branch from d32f5f9 to 91ca5ad Compare July 13, 2021 08:22
@wasfree
Copy link
Contributor Author

wasfree commented Jul 13, 2021

Thanks @wasfree - this looks good except for a failing tests:

------- Stdout: -------
=== RUN   TestAccPostgresqlFlexibleServerDatabase_requiresImport
=== PAUSE TestAccPostgresqlFlexibleServerDatabase_requiresImport
=== CONT  TestAccPostgresqlFlexibleServerDatabase_requiresImport
    testcase.go:89: Step 2/2, expected an error with pattern, no match on: Error running pre-apply refresh: exit status 1
        
        Error: Duplicate resource "azurerm_postgresql_flexible_server_database" configuration
        
          on terraform_plugin_test.tf line 35:
          35: resource "azurerm_postgresql_flexible_server_database" "test" {
        
        A azurerm_postgresql_flexible_server_database resource named "test" was
        already declared at terraform_plugin_test.tf:27,1-62. Resource names must be
        unique per type in each module.
    testing_new.go:63: Error retrieving state, there may be dangling resources: exit status 1
        
        Error: Duplicate resource "azurerm_postgresql_flexible_server_database" configuration
        
          on terraform_plugin_test.tf line 35:
          35: resource "azurerm_postgresql_flexible_server_database" "test" {
        
        A azurerm_postgresql_flexible_server_database resource named "test" was
        already declared at terraform_plugin_test.tf:27,1-62. Resource names must be
        unique per type in each module.
        
--- FAIL: TestAccPostgresqlFlexibleServerDatabase_requiresImport (377.32s)
FAIL

Fixed typo in Import. Thanks

@katbyte katbyte modified the milestones: v2.68.0, v2.69.0, v2.70.0 Jul 16, 2021
@wasfree wasfree force-pushed the f/postgresql-flex-database branch 2 times, most recently from c1f5bc0 to 519390f Compare July 24, 2021 08:04
@wasfree wasfree requested a review from katbyte July 26, 2021 06:18
@wasfree
Copy link
Contributor Author

wasfree commented Jul 26, 2021

@katbyte this PR should be ready to merge. Please let me know if you have any concerns.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

@wasfree - ive tried running the tests a number of time sbut they all fail with:

------- Stdout: -------
=== RUN   TestAccPostgresqlFlexibleServerDatabase_basic
=== PAUSE TestAccPostgresqlFlexibleServerDatabase_basic
=== CONT  TestAccPostgresqlFlexibleServerDatabase_basic
    testcase.go:88: Step 1/2 error: Error running apply: exit status 1
        
        Error: waiting for creation of Azure Postgresql Flexible Server database "acctest-fsd-210726033937888521" on server "acctest-fs-210726033937888521" (Resource Group "acctestRG-postgresql-210726033937888521"): Code="InternalServerError" Message="An unexpected error occured while processing the request. Tracking ID: '0d064330-9d2c-483f-b686-4b024d173bfa'"
        
          with azurerm_postgresql_flexible_server_database.test,
          on terraform_plugin_test.tf line 26, in resource "azurerm_postgresql_flexible_server_database" "test":
          26: resource "azurerm_postgresql_flexible_server_database" "test" {
        
--- FAIL: TestAccPostgresqlFlexibleServerDatabase_basic (577.57s)
FAIL

is this something your seeing?

@yupwei68
Copy link
Contributor

Hi @wasfree, kt, Tom, there is a bug with database. The database doesn't accept the flexible server with login name with capital letters. The service team is fixing this. The global release is about to be 5 days according to our last talk.

@wasfree
Copy link
Contributor Author

wasfree commented Jul 27, 2021

Hi @wasfree, kt, Tom, there is a bug with database. The database doesn't accept the flexible server with login name with capital letters. The service team is fixing this. The global release is about to be 5 days according to our last talk.

@yupwei68 interesting, is this something new? I'm not stumbled before over this issue, but your right if we don't use capital letters for administrator login in postgres flexserver resource the flexserver database resource tests pass without any issues.

@yupwei68
Copy link
Contributor

@wasfree I was tracking the optional fields problems, they said this was the cause.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

given that is a issue that should be fixed i'd honestly like a test to check that is 100% fixed and ensure going forward that bug doesn't pop up again (as i've seen regressions like that with other database APIs)

@katbyte katbyte modified the milestones: v2.70.0, v2.71.0 Jul 30, 2021
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @wasfree

Thanks for pushing those changes - taking a look through here on the whole this is looking pretty good, if we can fix up the comments I've left inline then this should be good to go 👍

Thanks!

Comment on lines +92 to +103
func (PostgresqlFlexibleServerDatabaseResource) charsetLowercase(data acceptance.TestData) string {
return fmt.Sprintf(`
%s

resource "azurerm_postgresql_flexible_server_database" "test" {
name = "acctest-fsd-%d"
server_id = azurerm_postgresql_flexible_server.test.id
collation = "en_US.latin1"
charset = "latin1"
}
`, PostgresqlFlexibleServerResource{}.basic(data), data.RandomInteger)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw this can become:

Suggested change
func (PostgresqlFlexibleServerDatabaseResource) charsetLowercase(data acceptance.TestData) string {
return fmt.Sprintf(`
%s
resource "azurerm_postgresql_flexible_server_database" "test" {
name = "acctest-fsd-%d"
server_id = azurerm_postgresql_flexible_server.test.id
collation = "en_US.latin1"
charset = "latin1"
}
`, PostgresqlFlexibleServerResource{}.basic(data), data.RandomInteger)
}
func (r PostgresqlFlexibleServerDatabaseResource) charsetLowercase(data acceptance.TestData) string {
return fmt.Sprintf(`
%s
resource "azurerm_postgresql_flexible_server_database" "test" {
name = azurerm_postgresql_flexible_server.test.name
server_id = azurerm_postgresql_flexible_server.test.id
collation = azurerm_postgresql_flexible_server.test.collation
charset = azurerm_postgresql_flexible_server.test.charset
}
`, r.basic(data))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't work we need PostgresqlFlexibleServerResource for base resources.

@wasfree wasfree force-pushed the f/postgresql-flex-database branch from c8050cc to 0588e92 Compare August 4, 2021 16:03
@wasfree
Copy link
Contributor Author

wasfree commented Aug 4, 2021

@tombuildsstuff rebased and comments resolved. There is only one question left regarding change of charsetLowercase test.

@yupwei68
Copy link
Contributor

yupwei68 commented Aug 5, 2021

Hi @wasfree The fix has been released globally. I have just tested them with success.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @wasfree and @yupwei68 - LGTM 🍰

@katbyte katbyte merged commit d12d202 into hashicorp:master Aug 5, 2021
katbyte added a commit that referenced this pull request Aug 5, 2021
@github-actions
Copy link

github-actions bot commented Aug 6, 2021

This functionality has been released in v2.71.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented Sep 6, 2021

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants