Always use /setup-tests endpoint for plan-only attributes#509
Draft
fivetran-jovanmanojlovic wants to merge 10 commits intomainfrom
Draft
Always use /setup-tests endpoint for plan-only attributes#509fivetran-jovanmanojlovic wants to merge 10 commits intomainfrom
fivetran-jovanmanojlovic wants to merge 10 commits intomainfrom
Conversation
Check plan-only attributes first and call /setup-tests endpoint before handling other updates via PATCH. This ensures setup tests run even when other attributes change in the same update. RD-1080143
Test will be added in separate PR after this fix is merged
44f7680 to
e32680d
Compare
| updatePerformed := false | ||
|
|
||
| if planOnlyAttributesChanged { | ||
| response, err := r.GetClient().NewConnectionSetupTests().ConnectionID(state.Id.ValueString()).DoCustom(ctx) |
Contributor
There was a problem hiding this comment.
In a situation when TF Config is having changes in both run_setup_tests and config, currently it seems it will end up in API requests sequence as:
POST /connections/{}/testPATCH /connections/{}with{ "config": {"password": "new-password"} },
i.e. setup tests are going to be performed on old config.
If this is so, looks like we need to switch the order - first patch connection, then run tests, as:
if hasUpdates {
...
r.GetClient().NewConnectionUpdate()
...
}
if planOnlyAttributesChanged {
...
r.GetClient().NewConnectionSetupTests()
...
}
| } | ||
|
|
||
| if planOnlyAttributesChanged { | ||
| response, err := r.GetClient().NewConnectionSetupTests().ConnectionID(state.Id.ValueString()).DoCustom(ctx) |
Contributor
There was a problem hiding this comment.
Seems we need to add trust-certificates and trust-fingerprints flags, like:
Suggested change
| response, err := r.GetClient().NewConnectionSetupTests().ConnectionID(state.Id.ValueString()).DoCustom(ctx) | |
| response, err := r.GetClient().NewConnectionSetupTests(). | |
| ConnectionID(state.Id.ValueString()). | |
| TrustCertificates(trustCertificates...). | |
| TrustFingerprints(trustFingerprints...). | |
| DoCustom(ctx) |
| updatePerformed = true | ||
| } | ||
|
|
||
| if planOnlyAttributesChanged { |
Contributor
There was a problem hiding this comment.
Looks like we should not call /connection/{}/test if it is set to false in TF Config.
And have condition
- either
if planOnlyAttributesChanged && runSetupTestsToPreserve {
- or if we're not calling tests in
PATCH /connection/{}then probably
if (planOnlyAttributesChanged || updatePerformed) && runSetupTestsToPreserve {
Contributor
There was a problem hiding this comment.
Edit - updated condition suggestion
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why Terraform Provider Didn't Call the Setup Tests Endpoint
The Terraform provider has two different paths for updating connectors:
Path 1 - PATCH endpoint: Used when "real" attributes change (like config, sync_frequency, paused, etc.)
Path 2 - /setup-tests endpoint: Used when ONLY plan-only attributes change (run_setup_tests, trust_certificates, trust_fingerprints)
The provider checks: "Are there any real attribute changes?"
Customer's scenario:
They changed BOTH:
Since there were "other" changes, the provider chose Path 1 (PATCH endpoint). The PATCH endpoint accepts the run_setup_tests parameter but the API ignores it unless there's also a config change.
Why API Doesn't Run Tests on PATCH Without Config Changes
The backend API has intentional logic in the PATCH endpoint:
Step 1: Check if config or auth fields changed
Step 2: If yes → Apply credential changes and run setup tests
Step 3: If no → Skip the entire setup test execution flow
Customer's PATCH request included:
The API checked: "Did config or auth change?"
This is intentional API design because setup tests are meant to validate configuration. If no configuration changed, the API assumes there's nothing new to test.
The separate /setup-tests endpoint exists for exactly this use case: Running tests without requiring a config change.
The Fix:
The Terraform provider needs to always check if plan-only attributes changed FIRST, and call /setup-tests endpoint when needed, THEN handle any other attribute changes via PATCH. This ensures setup tests run regardless of what other attributes changed in the same update.