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-4606: Handle browser check defaults #65

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

Pajk
Copy link
Collaborator

@Pajk Pajk commented Nov 7, 2024

Replaces #64
Resolves #38
Resolves #59
Resolves #21

Requires splunk/syntheticsclient#30


Before the change?

  • The default value of wait_for_nav_timeout is always 50 in the provider but the server-side default is conditional. It's 2000 when wait_for_nav=true and 50 when wait_for_nav=false. So it's currently causing issues when wait_for_nav is true in the configuration but the wait_for_nav_timeout is not set.
  • For the max_wait_time it's working well because the default value is static (always 10000).
  • Default value of max_wait_time and wait_for_nav_timeout is set automatically for all test types by the provider. Although the API do ignore them for steps where these fields are not applicable, it's not ideal because TF state does not reflect the state on the server.
  • Terraform diff for the resource synthetics_create_browser_check_v2 is useless. Any change on the server or in the configuration triggers full replacement of the test and it's not clearly visible in the diff what values are going to be modified.

After the change?

  • Default values are set by API, not by the provider. The provider is not keeping the server-side default values in the TF state to avoid differences between the resource configuration and TF state. The default values are not hardcoded in the provider, the new wait_for_nav_timeout_default and max_wait_time_default boolean flags are used to find out wether the value of the respective fields is a custom value or it's just the default provided by the Synthetics API.

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

$ TF_ACC=1 TF_LOG=DEBUG go test ./synthetics/... -run TestAccCreateUpdateBrowserCheckV2 
ok      github.com/splunk/terraform-provider-synthetics/synthetics      8.711s

Does this introduce a breaking change?

  • Yes
  • No

User will see some changes in the Terraform plan -> default values will be removed from the state for tests where they are not applicable.


Copy link

github-actions bot commented Nov 7, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Pajk
Copy link
Collaborator Author

Pajk commented Nov 8, 2024

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

@Pajk
Copy link
Collaborator Author

Pajk commented Nov 8, 2024

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

@Pajk Pajk marked this pull request as ready for review November 13, 2024 14:26
Copy link
Collaborator

@sangn-splunk sangn-splunk left a comment

Choose a reason for hiding this comment

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

👏 wow this looks good!

@Pajk Pajk force-pushed the SYN-4606-browser-check-defaults branch from 9aaf074 to 59d0afc Compare November 15, 2024 10:24
@Pajk Pajk merged commit b19c73c into splunk:main Nov 19, 2024
4 checks passed
@Pajk Pajk deleted the SYN-4606-browser-check-defaults branch November 19, 2024 15:37
@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2024
@greatestusername-splunk
Copy link
Collaborator

Thank you for doing this! Awesome work!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants