postgresql_server: Update a PostgreSQL Replica to create_mode = "Default"

[aristosvo]

Fixes #11330

Acceptance tests

• New acceptance test for new functionality: TestAccPostgreSQLServer_updateReplicaToDefault

  ❯ make acctests SERVICE='postgres' TESTARGS='-run=updateReplicaToDefault'
  ==> Checking that code complies with gofmt requirements...
  ==> Checking that Custom Timeouts are used...
  ==> Checking that acceptance test packages are used...
  TF_ACC=1 go test -v ./azurerm/internal/services/postgres -run=updateReplicaToDefault -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
  2021/04/25 23:41:55 [DEBUG] not using binary driver name, it's no longer needed
  2021/04/25 23:41:56 [DEBUG] not using binary driver name, it's no longer needed
  === RUN   TestAccPostgreSQLServer_updateReplicaToDefault
  === PAUSE TestAccPostgreSQLServer_updateReplicaToDefault
  === CONT  TestAccPostgreSQLServer_updateReplicaToDefault
  --- PASS: TestAccPostgreSQLServer_updateReplicaToDefault (884.38s)
  PASS
  ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/postgres    887.796s

[katbyte]

Aside from one comment this looks good 🚀 thanks @aristosvo !

[katbyte]

we'll need to nil check ReplicationRole to prevent panics

[aristosvo]

Thanks, done!

[katbyte]

Also most tests are failing with:

------- Stdout: -------
=== RUN   TestAccPostgreSQLServer_basicEleven
=== PAUSE TestAccPostgreSQLServer_basicEleven
=== CONT  TestAccPostgreSQLServer_basicEleven
    testing_new_import_state.go:249: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) (len=1) {
         (string) (len=25) "creation_source_server_id": (string) ""
        }
        
        
        (map[string]string) {
        }
--- FAIL: TestAccPostgreSQLServer_basicEleven (393.28s)
FAIL

------- Stderr: -------
2021/04/26 01:01:46 [DEBUG] not using binary driver name, it's no longer needed
2021/04/26 01:01:47 [DEBUG] not using binary driver name, it's no longer needed

[aristosvo]

Also most tests are failing with:

------- Stdout: -------
=== RUN   TestAccPostgreSQLServer_basicEleven
=== PAUSE TestAccPostgreSQLServer_basicEleven
=== CONT  TestAccPostgreSQLServer_basicEleven
    testing_new_import_state.go:249: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) (len=1) {
         (string) (len=25) "creation_source_server_id": (string) ""
        }
        
        
        (map[string]string) {
        }
--- FAIL: TestAccPostgreSQLServer_basicEleven (393.28s)
FAIL

------- Stderr: -------
2021/04/26 01:01:46 [DEBUG] not using binary driver name, it's no longer needed
2021/04/26 01:01:47 [DEBUG] not using binary driver name, it's no longer needed

Oops, my mistake. I took the gamble as I had no credits left in my Azure Visual Studio Enterprise Subscription to role the whole suite.

Edit:
I expected that setting creation_source_server_id = null on the new test to should fix it instead of d.Set("creation_source_server_id", "") on import, but the new test is now hanging.

I'm not sure how to fix this, other than ignoring the diff between import and state for creation_source_server_id. From a functionality point of view this is not a problem, as creation_source_server_id is only used during creation of the PostgreSQL Server.

[mybayern1974]

Hi @aristosvo ,

Thank you for raising this PR! Given I’m also subscribing the issue 11330, I’d like to share my 2 cents though I believe this fix is able to solve this issue.

1. At the service backend, there are two parameters CreateMode and ReplicationRole. The former only serves CREATE/PUT and the latter only serves UPDATE/PATCH. Given what the issue wants is the latter, will it be more clear to directly expose the latter property rather than making the magic happen through making the former as a wrapper?
2. The purpose of the issue is to make both the Primary and Replica server become Standalone server by passing ReplicationRole=None to the backend. Directly exposing the ReplicationRole may help make users clear on what they are doing, while exposing CreateMode=Replica ==> CreateMode=Default as this PR does may bring the unclear to some users: What does Default mode mean?
3. Despite of which implementation solution would finally be, suggest to add below changes in doc and codes:

• Tell users that passing ReplicationRole=None is irreversible.
• In the code, add check that if in-update() && ReplicationRole=None && original-mode-is-replica, users must erase creation_source_server_id in their tf config, because at the service backend, when ReplicationRole=None gets passed to the backend, the service will erase the creation_source_server_id in the backend which will bring state inconsistency between client and service. This concern indeed has been detected by the regression testing result provided by @ Kt. Besides adding this detection in code, suggest also mention it in the doc.

As for follow ups, I would appreciate if the PR could be tuned per above suggest or I would be happy to submit a PR embodying the above if we can reach agreement here. I do not object to the current solution, either, if we agree on it, which could also solve the issue, as long as necessary in-code check and doc gets enriched.

Lastly, I cannot tell for now whether there would be other problems before we running full regression testing.

[aristosvo]

Hi @mybayern1974!

Thanks for your comments, like it! Have thought about exposing replication_role, but it didn't fit in my Desired State-thinking to have a separate flag for creation and updates 🤣 I believe it may be better to expose that extra variable, but it will be confusing as well, I'd prefer a better API from Azure side :)

I'll for sure add some extra docs! Thanks for the pointers in that regard.

In the code, add check that if in-update() && ReplicationRole=None && original-mode-is-replica, users must erase creation_source_server_id in their tf config, because at the service backend, when ReplicationRole=None gets passed to the backend, the service will erase the creation_source_server_id in the backend which will bring state inconsistency between client and service. This concern indeed has been detected by the regression testing result provided by @ Kt. Besides adding this detection in code, suggest also mention it in the doc.

With regard to your last bullet point, can you explain what you mean? creation_source_server_id is only used on creation time, so there is always state inconsistency if this flag is filled after creation. It also looks like this state variable cannot be removed completely (causing me trouble in the diff between import and state), but that is a minor problem in my opinion.

[katbyte]

thanks @aristosvo - tests pass now so i'm going to approve and merge this so the fix makes it into this weeks release and any additional fixes can be done in a subsequent pr. I think it's reasonable to expect going to expect that diff there and while not ideal agree its minor

[aristosvo]

@mybayern1974 Feel free to create another PR to expose replication_role as this one is merged.

Can you also pick up the docs from there, as it is double the effort if I do it for current workings as well!?

[ghost]

This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.57.0"
}
# ... other configuration ...

[github-actions]

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.