chore: Use timeouts attributes in autogen resources #3817

lantoli · 2025-10-25T16:18:36Z

Description

Use timeouts attributes in autogen resources.

Link to any related issue(s): CLOUDP-352321

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR. A migration guide must be created or updated if the new feature will go in a major version.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contributing guides
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals I have added appropriate changelog entries.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

Copilot

Pull Request Overview

This PR refactors autogenerated resource handlers to use timeout attributes from Terraform configurations instead of hard-coded timeout values. The changes move error checking into the shared handler functions and replace the TimeoutSeconds field with a Timeout field of type time.Duration.

Key changes:

Shared handler functions now perform early error checking via HasError() calls
Resources with wait configurations now extract timeout values from plan/state using the Terraform framework's timeout methods
The WaitReq struct's TimeoutSeconds integer field is replaced with a Timeout field of type time.Duration

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tools/codegen/gofilegen/resource/testdata/wait-configuration.golden.go	Test data updated to demonstrate timeout extraction from plan/state and removal of early error checks
tools/codegen/gofilegen/resource/testdata/update-with-put.golden.go	Test data showing removal of early error checks in handlers
tools/codegen/gofilegen/resource/testdata/static-request-body-delete.golden.go	Test data showing removal of early error checks in handlers
tools/codegen/gofilegen/resource/testdata/no-op-delete-operation.golden.go	Test data showing removal of early error checks in handlers
tools/codegen/gofilegen/resource/testdata/different-urls-with-path-params.golden.go	Test data showing removal of early error checks in handlers
tools/codegen/gofilegen/codetemplate/resource-file.go.tmpl	Template updated to generate timeout extraction code and use `Timeout` field instead of `TimeoutSeconds`
internal/serviceapi/streamprocessorapi/resource.go	Resource updated to extract timeouts and pass `time.Duration` values
internal/serviceapi/streaminstanceapi/resource.go	Resource updated to remove early error checks
internal/serviceapi/searchdeploymentapi/resource.go	Resource updated to extract timeouts and pass `time.Duration` values
internal/serviceapi/resourcepolicyapi/resource.go	Resource updated to remove early error checks
internal/serviceapi/pushbasedlogexportapi/resource.go	Resource updated to extract timeouts and pass `time.Duration` values
internal/serviceapi/projectsettingsapi/resource.go	Resource updated to remove early error checks
internal/serviceapi/projectapi/resource.go	Resource updated to remove early error checks
internal/serviceapi/orgserviceaccountapi/resource.go	Resource updated to remove early error checks
internal/serviceapi/maintenancewindowapi/resource.go	Resource updated to remove early error checks
internal/serviceapi/databaseuserapi/resource.go	Resource updated to remove early error checks
internal/serviceapi/customdbroleapi/resource.go	Resource updated to remove early error checks
internal/serviceapi/clusterapi/resource.go	Resource updated to extract timeouts and pass `time.Duration` values
internal/serviceapi/auditingapi/resource.go	Resource updated to remove early error checks
internal/common/autogen/handle_operations.go	Shared handler functions updated to check for errors early and accept `time.Duration` for timeout

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/common/autogen/handle_operations.go

tools/codegen/gofilegen/codetemplate/resource-file.go.tmpl

EspenAlbert · 2025-10-27T07:53:25Z

internal/serviceapi/streamprocessorapi/resource.go

 func (r *rs) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
 	var plan TFModel
 	resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
-	if resp.Diagnostics.HasError() {


Why are we removing this?

as in the PR description, we are moving it to the beginning of the handle methods like HandleCreate, etc.
No need to check it in each single resource when it can be done in the common code

I see, so the line plan.Timeouts.Create(ctx, 300*time.Second) will not have some weird error if this fails earlier?

uhm, let me try that

simulated that setting the plan fails, it works fine because timeouts just get the default one, and then HandleCreate checks HasErrors in the beginning and displays the error

tools/codegen/gofilegen/resource/testdata/wait-configuration.golden.go

EspenAlbert

Just a few questions. I think it makes the code a bit less readable, but no strong opinion.

manupedrozo · 2025-10-27T08:18:10Z

internal/serviceapi/auditingapi/resource.go

-	if resp.Diagnostics.HasError() {
-		return
-	}


I appreciate the effort to reduce template size. In this case I think its clearer to leave this check here right after the Plan.Get() call instead of waiting until the create/update/read operation to check the diags.

I understand that following this approach would also mean adding a check for the timeout creation.
Given that checking errors after a function call is a typical go pattern, I think its ok to generate some extra code for it.

No strong feelings either way, we can always go back to generating checks if necessary.

I think it's clearer just to set all the input params for handle operations, and the doing the "real" logic including error checks in handler functions.
Good thing about diagnostics is that it keeps a list of errors, warning, etc. so we don't lose information even if we accumulate some of them and we don't do HasErrors every time.

as in this comment, there is no need to do additional checks as timeouts won't fail (it will just get the default value), but if both @manupedrozo @EspenAlbert prefer keeping the check, I'll do that, reverting those changes.

timeouts never fails (it only logs info sometimes in current implementation) but will also add hasErrors to follow the same pattern

Feel free to keep it; no strong opinion, and in general, the handler functions will have better test coverage.
Anything added to the resources before calling the handler function must ensure that they can handle default values.

I think current approach has better tradeoffs (keeping resources just with input params and move away logic that is common) but also think it's important to get consensus, reverted here: e903635

* master: chore: Invoke Delete operation if timeout in autogen (#3820) Add support for typed nested sets in autogen (#3819) chore: Use timeouts attributes in autogen resources (#3817) refactor: Adds invalid update error for delete_on_create_timeout for sdkv2 (#3810) Add support for typed lists in autogen (#3816) refactor: Uses delete_on_create_timeout with default=true support across TPF resources (#3809) Rename customtype package to customtypes (#3814) chore: Add support for typed nested lists in autogen (#3813) don't schedule matrix until CLOUDP-353513 (#3812) chore: Fix stream tests notifications (#3805) chore: Bump golang.org/x/oauth2 from 0.31.0 to 0.32.0 (#3803) depends on data sources (#3808) test: Avoids flaky test by simplifying the check, ensuring count > 0 instead of reading API response and trying exact match (#3806) chore: Generate timeouts attribute for autogen resources (#3802) # Conflicts: # .github/workflows/acceptance-tests-runner.yml

lantoli added 6 commits October 25, 2025 18:16

template

1e6739a

generate resources

512627d

runtime

e45e2e4

reduce template code

1595a4e

remove HasError from template

fbcae67

fix golden files

970f669

lantoli marked this pull request as ready for review October 27, 2025 07:31

lantoli requested a review from a team as a code owner October 27, 2025 07:31

Copilot AI review requested due to automatic review settings October 27, 2025 07:31

Copilot AI reviewed Oct 27, 2025

View reviewed changes

internal/common/autogen/handle_operations.go Show resolved Hide resolved

tools/codegen/gofilegen/codetemplate/resource-file.go.tmpl Show resolved Hide resolved

EspenAlbert reviewed Oct 27, 2025

View reviewed changes

tools/codegen/gofilegen/resource/testdata/wait-configuration.golden.go Show resolved Hide resolved

EspenAlbert approved these changes Oct 27, 2025

View reviewed changes

manupedrozo approved these changes Oct 27, 2025

View reviewed changes

revert changes to have HasErrors checks in resources

e903635

lantoli merged commit 979ca9c into master Oct 27, 2025
47 checks passed

lantoli deleted the CLOUDP-352321_timeout_use branch October 27, 2025 09:56

chore: Use timeouts attributes in autogen resources #3817

chore: Use timeouts attributes in autogen resources #3817

Uh oh!

Conversation

lantoli commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change:

Required Checklist:

Further comments

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lantoli Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EspenAlbert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lantoli Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lantoli commented Oct 25, 2025 •

edited

Loading

lantoli Oct 27, 2025 •

edited

Loading

lantoli Oct 27, 2025 •

edited

Loading