confluentinc / terraform-provider-confluent

Terraform Provider for Confluent
Apache License 2.0
118 stars 61 forks source link

fix: Update state when refreshing schema #318 #293 #322

Closed Noel-Jones closed 1 month ago

Noel-Jones commented 8 months ago

Gets and sets the schema within readSchemaRegistryConfigAndSetAttributes so it will be consistently actioned. This ensures that when the resource is refreshed the state file is updated (for #318). Removed corrsponding code from immediately after other calls (for import and data resource). As a result of this the use of SCHEMA_CONTENT during import is removed, which I cannot fathom a use for as imho an import should always be from the actual resource and not a local data file (#293).

I have used the provider in dev mode to make updates (driven by the fixed refresh), perform an import and verify the data resource. The existing tests all run without apparant issue.

Grateful for review, comments and assistance with modifying the acceptance tests.

cla-assistant[bot] commented 8 months ago

CLA assistant check
All committers have signed the CLA.

cla-assistant[bot] commented 8 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Noel-Jones commented 8 months ago

That was a fun but educational week looking at how the acceptance tests work and working out how to build a test scenario to fit the case. I finally got there with a test that fails with the existing code and succeeds with this change.

Grateful for code owner review because this is really holding me using Terraform to manage our production environment.

linouk23 commented 8 months ago

@Noel-Jones thank you so much for creating this PR! We'll take a look in a bit.

linouk23 commented 2 months ago

@Noel-Jones thank you so much for waiting!

Our repository isn't set up to accept community contributions, but we merged this fix internally, ran some manual tests too, and it does look very promising so far, so we'll include it in our next TF release.

Noel-Jones commented 2 months ago

Thanks Kostya, I appreciate the update and feedback,

ConfluentSpencer commented 1 month ago

Merged fix with internal PR. Will close this issue. https://github.com/confluentinc/terraform-provider-confluent/issues/293

linouk23 commented 2 weeks ago

@Noel-Jones fyi seems like caused some issues for other users, see #378.

Noel-Jones commented 2 weeks ago

Hi Kostya. Sorry to see that.

For that issue (378) before 1.70 terraform would have compare the file contents to the state contents which would have matched (effectively Terraform checking the supplied schema is the same as the one originally supplied, but with no reference to what is in the schema registry). After 1.70 it would compare the actual schema from the registry against the file contents, which is the correct thing to do. The observed issue is that the spacing (and line endings) is dropped in the actual schema created in or returned from the schema registry (proven when Alex compresses the state entry). I think that because create_on_update is true the apparent change in the schema is being picked up by the provider and we see the error message.

I can try and reproduce and further investigate when time allows but cannot give a timeframe.