Skip to content
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.

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

Adds customizable Timeouts to resources/data that rely on syncing users and groups to avoid context.DeadlineExceeded #1207

Merged
merged 3 commits into from
Aug 25, 2022

Conversation

emanor-okta
Copy link
Contributor

[WIP] Adds customizable Timeouts to resources/data that rely on syncing users and groups (https://www.terraform.io/plugin/sdkv2/resources/retries-and-customizable-timeouts)

Changes the default timeouts from 20min to 1 year (no timeout) for application resources that can sync user/group memberships. This should resolve issues with running into context.DeadlineExceeded during the plan/apply phase for Orgs/Apps with large user assignments that don't use federation broker mode.

Addresses issue #1101

@monde monde self-requested a review July 19, 2022 17:35
@monde
Copy link
Collaborator

monde commented Jul 19, 2022

Thanks for the PR @emanor-okta . I'll be able to look at this closer later in the week.

@monde
Copy link
Collaborator

monde commented Jul 28, 2022

@emanor-okta disabling timeouts on all the C/R/U's of CRUD doesn't feel right to me. If that was the proper behavior then the TF SDK would ship with that as its default. Instead, this PR is a substantial behavior change and flips the onus on to the operator to set timeouts in their config; perhaps this would be considered a breaking change to some. Also touching 30 files is not DRY at all.

@emanor-okta
Copy link
Contributor Author

@monde - It would be possible to also make the timeout value be 20 minutes schema.DefaultTimeout(20 * time.Minute).
This way it matches the current timeout and the config option would still be available for these resources so an operator could modify it if they ran into issues with timeouts.

@emanor-okta
Copy link
Contributor Author

@monde - I removed the Timeout block from the data_source config files and website files.
For the resource files that make use of the sync user/groups the read/create/delete default timeout has been set to 1 hour which should be enough for most use cases and can always be modified via .tf

@monde monde changed the title [WIP] Adds customizable Timeouts to resources/data that rely on syncing users and groups to avoid context.DeadlineExceeded Adds customizable Timeouts to resources/data that rely on syncing users and groups to avoid context.DeadlineExceeded Aug 25, 2022
@monde
Copy link
Collaborator

monde commented Aug 25, 2022

I wrote the ACC tests for this #1267

Copy link
Collaborator

@monde monde left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants