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

SYN-4498: Fix issues with default values #64

Closed
wants to merge 4 commits into from

Conversation

Pajk
Copy link
Collaborator

@Pajk Pajk commented Oct 22, 2024

Before the change?

  • Default values for max_wait_time and wait_for_nav_timeout are defined and used for all types of steps
  • This could lead to unexpected behavior:

A) Value gets persisted in the database. That could cause validation issues - eg. in case the API introduces stricter validation.
B) Value is not persisted because the API drops it. Terraform always reports drift in the plan because the value from API differs from the configuration.

  • In case the value in the database is set to 0 user is forced to change the value to a different value than the default one. The API returns 0 but the provider replaces it with the default value and reports empty plan. This happened because of a bug in v2.0.9 of this provider.

After the change?

  • There are no default values for max_wait_time and wait_for_nav_timeout in the provider
  • The default value is not being tracked in the TF state - it's ignored
  • (not ideal) Users can't set these fields to their default values. There's a validation and the user is informed they need to set it to a different value. Since these values are ignored and not persisted to the TF state the plan would always report a change when set to the default value in the resource configuration.

I tried hard to remove this limitations but the support in terraform-plugin-sdk for working with TypeSet and ListSet is probably not sufficient. I tried all kind of approaches - Optional Computed attribute, DiffSuppressFunc, CustomizeDiff... but I was never able to cover all cases.

I haven't found a way how to ignore the value in configuration when it's set to the default value. It might be possible with the PlanModifiers when using the new terraform-plugin-framework but I was not able to modify the plan in CustomizeDiff - I was not even able to access the transactions in the new value, it was always nil.

Pull request checklist

  • Acceptance Tests have been updated, run (make testacc), and pasted in this PR (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Acceptance Test Output


Does this introduce a breaking change?

  • Yes
  • No

Upgrading from any version:

  • If max_wait_time or wait_for_nav_timeout is used and set to a non default value, no changes are needed and there are no changes in state or in the plan.

If users upgrade from older version than v2.0.9:

  • BC: If max_wait_time is set to 10000 or wait_for_nav_timeout to 50 or 2000 the validation fails and user has to remove it from the configuration.
  • If max_wait_time or wait_for_nav_timeout is not used in the configuration, no user action is required and there are no changes in state or in the plan.

If upgraded from v2.0.9 or v2.0.10:

  • BC: If max_wait_time is set to 10000 or wait_for_nav_timeout to 50 or 2000 the validation will fail and user has to remove it from the configuration.
  • If there're 0 values in the database and the attributes are not configured in the TF resource, no change is triggered. This has to be still fixed manually - by removing the value from the database or by setting the field to some non default value in terraform config. To fix this scenario I think we can just change the API behavior and return the default value instead 0 even when the 0 is persisted in the test config.
  • If max_wait_time or wait_for_nav_timeout is not used in the configuration, no user action is required but there there will be changes in the TF state (default -> 0) so the plan will not be empty.

I don't think this is an ideal solution and I'm open to any suggestion how to handle this situation properly.

Other option I was thinking about was to keep the default values in the state but mirror the logic from the API to the provider and set the default only to specific test types (eg. max_wait_time for assert_* types). I didn't want to add additional logic to the provider but it might be a better solution after all.

Copy link

github-actions bot commented Oct 22, 2024

CLA Assistant Lite bot: All contributors have NOT signed the COC Document


I have read the Code of Conduct and I hereby accept the Terms


You can retrigger this bot by commenting recheck in this Pull Request

@Pajk
Copy link
Collaborator Author

Pajk commented Oct 22, 2024

I have read the CLA Document and I hereby sign the CLA

srv-gh-tools added a commit to splunk/cla-agreement that referenced this pull request Oct 22, 2024
@Pajk Pajk closed this Nov 5, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2024
@Pajk Pajk deleted the SYN-4498-max-wait-time-default branch November 6, 2024 10:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant