fix: correct publication import ID regenerated before name is set - #1
Open
brtkwr wants to merge 1 commit into
Open
fix: correct publication import ID regenerated before name is set#1brtkwr wants to merge 1 commit into
brtkwr wants to merge 1 commit into
Conversation
resourcePostgreSQLPublicationReadImpl regenerated the resource ID from the name attribute before setting it, so importing a publication produced a mangled 'database.' ID with an empty publication name. Set the name attribute first, then regenerate the ID, and add an ImportStateVerify step to the schemas acceptance test to cover the import round-trip.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on top of your
add-publication-schemasbranch (base is your branch, notmain), so this diff is just the delta: a one-line fix plus an import test.The bug
The
postgresql_publicationresource has a latent import bug (pre-existing onmain, inherited by this branch): inresourcePostgreSQLPublicationReadImplthe ID is regenerated from thenameattribute before that attribute is written to state, so on importgeneratePublicationIDreads an empty name and produces a malformed"<database>."ID. It breaksImportStateVerifyround-trips.The change
resource_postgresql_publication.go: moved.Set(pubNameAttr, PublicationName)aboved.SetId(generatePublicationID(d, database))so the name is in state before the ID is regenerated (with a short why comment).resource_postgresql_publication_test.go: add anImportState/ImportStateVerifystep to yourTestAccPostgresqlPublication_Schemastest. Schema-scoped publications are the newest thing people will import, so it is the natural place to cover it.Why here
The fix is orthogonal to your
schemasfeature, but it bites hardest there, and your test was the natural home for the import coverage. I reproduced the failure on your branch before the fix and confirmed green after:resource with ID tf_tests_db_<suffix>. not found(note the trailing dot, empty publication name).TestAccPostgresqlPublicationsuite on live PG15 (docker-compose harness,PGVERSION=15) passes 15, skips 2 (pre-11 truncate variants), 0 failures, including all five of your schema tests and the new import step.gofmtclean,go build ./...andgo vet ./postgresql/pass.Context: I had independently opened cyrilgdn#653 for the same
schemasfeature and carry this same fix there. Yours is further along, so I would rather help this one land than duplicate it. Whichever merges first, the other rebases cleanly. Happy to adjust anything.