feat: add getImportIDFn

[fernandezcuesta]

Description of your changes

Some Terraform providers store resource IDs in a different format than what their import function expects. Until now, upjet used a single function (GetIDFn) to produce the ID for both purposes, making it impossible to support these providers correctly.

This PR introduces an optional GetImportIDFn field to ExternalName. When set, upjet uses it to produce the ID passed to terraform import, while continuing to use GetIDFn for the state file used by terraform refresh. When unset, the existing behavior is preserved.

Fixes #676

I have:

• Read and followed Upjet's contribution process.
• Run make reviewable to ensure this PR is ready for review.
• [ ] Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Add unit tests.

Built provider-mongodbatlas 1.2.0-rc.1 on top of these changes, now I can set a custom function for GetImportIDFn for those resources where the upstream TF provider works as explained above.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds an optional GetImportIDFn field to the ExternalName struct, then updates WorkspaceStore.Workspace to compute tfStateID via GetIDFn, call EnsureTFState with that value, and—when GetImportIDFn is set—override w.terraformID with the import-specific ID format. Tests verify fallback behavior, override precedence, error handling, and state file persistence.

Changes

Separate Terraform Import ID from State ID

Layer / File(s)	Summary
ExternalName contract and Workspace implementation pkg/config/resource.go, pkg/terraform/store.go	GetImportIDFn GetIDFn field is added to ExternalName with docs noting nil falls back to GetIDFn. In Workspace, tfStateID is derived via GetIDFn and passed to EnsureTFState; w.terraformID is set to tfStateID but optionally overwritten by GetImportIDFn when configured. Both ID function errors are wrapped with errGetID.
Test helpers and import ID computation tests pkg/terraform/store_test.go (lines 1–173)	Test setup constructs a test resource, fake terraformed object, and workspace store with init disabled. Table-driven TestWorkspaceStoreGetImportIDFn validates fallback to GetIDFn when GetImportIDFn is nil, precedence when set, error propagation from both functions, and correct error wrapping.
State persistence verification pkg/terraform/store_test.go (lines 175–211)	TestWorkspaceStoreGetImportIDFnStateVsTerraformID executes Workspace with both ID functions, reads the generated terraform.tfstate, and asserts the state file contains the GetIDFn-derived ID but not the GetImportIDFn-derived import ID.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

---

Thank you for this contribution! A couple of clarifying questions to help ensure the implementation aligns with your intent:

• When GetImportIDFn returns an error in store.go, the error is wrapped with errGetID — is that intentional, or would a distinct sentinel (e.g., errGetImportID) be helpful for distinguishing which function failed during debugging or error logging?

• Could you share a concrete example of a provider where terraform import and terraform state require different ID formats (e.g., the MongoDB Atlas provider mentioned in issue #676)? That context would help reviewers understand the motivation, and whether any edge cases (e.g., both functions returning the same ID in practice) or gotchas deserve a note in the code or docs.

🚥 Pre-merge checks | ✅ 7

✅ Passed checks (7 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main feature being added: a new optional GetImportIDFn field to support different ID formats for Terraform import versus state.
Linked Issues check	✅ Passed	The PR fully addresses issue #676 by adding an optional GetImportIDFn field to ExternalName that allows separate ID formatting for terraform import while preserving GetIDFn for state files.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to implementing the GetImportIDFn feature: adding the field, using it in WorkspaceStore, and adding comprehensive test coverage.
Configuration Api Breaking Changes	✅ Passed	Added optional GetImportIDFn field to ExternalName struct. No breaking changes: no exports removed/renamed, no function signatures changed, fully backward compatible with fallback to GetIDFn when u...
Generated Code Manual Edits	✅ Passed	No manual edits to zz_.go files detected. The only zz_.go file (zz_generated.deepcopy.go) was newly auto-generated by controller-gen, not manually edited.
Template Breaking Changes	✅ Passed	No existing templates in pkg/controller/ or pkg/pipeline/templates/ were modified. All template files show "Added" status, not "Modified." The PR adds library functionality without altering templat...
Description check	✅ Passed	The PR description clearly explains the motivation (supporting Terraform providers with different ID formats), the solution (optional GetImportIDFn field), backward compatibility, and provides real-world testing context.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

pkg/terraform/store.go (1)

260-278: 💤 Low value

Thanks for implementing this separation cleanly!

The logic correctly ensures that:

1. tfStateID (from GetIDFn) is used for EnsureTFState — maintaining state consistency
2. w.terraformID can be overridden via GetImportIDFn — enabling different import formats

This aligns well with the downstream usage in workspace.go where w.terraformID is explicitly noted as the import ID.

One small suggestion for debugging ergonomics: both error paths (lines 263 and 274) wrap with the same errGetID message. If a provider developer hits an error, it might be tricky to know which function failed. Would you consider differentiating them? For example:

💡 Optional: Differentiate error messages

 tfStateID, err := fp.Config.ExternalName.GetIDFn(ctx, externalName, fp.parameters, fp.Setup.Map())
 if err != nil {
-    return nil, errors.Wrap(err, errGetID)
+    return nil, errors.Wrap(err, "cannot get state ID")
 }

 // ...
 if fp.Config.ExternalName.GetImportIDFn != nil {
     w.terraformID, err = fp.Config.ExternalName.GetImportIDFn(ctx, externalName, fp.parameters, fp.Setup.Map())
     if err != nil {
-        return nil, errors.Wrap(err, errGetID)
+        return nil, errors.Wrap(err, "cannot get import ID")
     }
 }

This is purely optional — the current approach is consistent with the existing pattern. What do you think?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/terraform/store.go` around lines 260 - 278, The error handling for
GetIDFn and GetImportIDFn both wrap errors with the same errGetID message,
making it difficult to distinguish which function failed during debugging. In
the error wrapping calls after GetIDFn (line 263) and GetImportIDFn (line 274),
use differentiated error messages for each path. For example, keep errGetID for
the GetIDFn error and create or use a separate error constant (such as
errGetImportID) for the GetImportIDFn error path to help provider developers
quickly identify which function failed.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/terraform/store.go`:
- Around line 260-278: The error handling for GetIDFn and GetImportIDFn both
wrap errors with the same errGetID message, making it difficult to distinguish
which function failed during debugging. In the error wrapping calls after
GetIDFn (line 263) and GetImportIDFn (line 274), use differentiated error
messages for each path. For example, keep errGetID for the GetIDFn error and
create or use a separate error constant (such as errGetImportID) for the
GetImportIDFn error path to help provider developers quickly identify which
function failed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 52f2ccf6-f4ec-437e-bae2-415036e32894

📥 Commits

Reviewing files that changed from the base of the PR and between 59c4552 and b2a38c0.

📒 Files selected for processing (2)

• pkg/config/resource.go
• pkg/terraform/store.go