Skip to content

Skip synthetic TF state creation when external identifier is empty #625

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.

Sign up for GitHub

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

Open
bitgandtter wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Skip synthetic TF state creation when external identifier is empty #625

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/terraform/files.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,10 @@ func (fp *FileProducer) EnsureTFState(_ context.Context, tfID string) error { //
// This is especially useful for resources whose deletion are scheduled for
// a long period of time, where if we fill the ID, the queries would actually
// succeed, i.e. GCP KMS KeyRing.
if !empty || meta.WasDeleted(fp.Resource) {
// Skip synthetic state creation when: state already exists, resource is
// being deleted, or there is no external identifier yet (new resource).
// Without an ID, Refresh would fail on providers that require it for Read.
if !empty || meta.WasDeleted(fp.Resource) || tfID == "" {
Comment on lines +245 to +248
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 30, 2026

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Please add (or point to) a regression test for the empty tfID guard

Nice fix—this guard is key for IdentifierFromProvider flows. Could you add or reference a table-driven unit test that covers: empty state + not deleted + tfID == "" should not write synthetic terraform.tfstate? That would lock the behavior and prevent future observe/create regressions.

As per coding guidelines: **/*.go: “All Upjet code must be covered by tests… Upjet encourages the use of table driven unit tests.”

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/terraform/files.go` around lines 245 - 248, Add a table-driven unit test
for the logic in pkg/terraform/files.go that contains the guard "if !empty ||
meta.WasDeleted(fp.Resource) || tfID == """: create cases that cover (1) empty
state + not deleted + tfID == "" and assert that no synthetic terraform.tfstate
file is written, and (2) complementary cases where state should be written. In
the test, instantiate a fake FileProcessor/FP (or the same test helper used
elsewhere), set fp.Resource so meta.WasDeleted(fp.Resource) returns false, set
empty=true and tfID="" for the critical case, run the function that evaluates
this guard (the function in files.go that writes synthetic state), and assert on
filesystem or in-memory writer that no file was created; repeat for cases where
a file should be created to lock behavior and prevent regressions.

return nil
}
base := make(map[string]any)
Expand Down