TRACKAI-2061: Pass-through unconfigurable connection options #786
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #786 +/- ##
=======================================
Coverage 86.70% 86.71%
=======================================
Files 83 83
Lines 12443 12514 +71
=======================================
+ Hits 10789 10851 +62
- Misses 1258 1264 +6
- Partials 396 399 +3
|
| @@ -555,6 +559,21 @@ func expandConnectionOptionsAD(config cty.Value) (*management.ConnectionOptionsA | |||
| BruteForceProtection: value.Bool(config.GetAttr("brute_force_protection")), | |||
| } | |||
|
|
|||
| if !d.IsNewResource() { | |||
| conn, err := api.Connection.Read(d.Id()) | |||
There was a problem hiding this comment.
We don't really need to pass the API here in all these places and force another request to fetch the current details. We can add all the extra properties as read-only (Computed), and simply update all the expands to fetch those details as well and pass them through, effectively skipping the API call.
There was a problem hiding this comment.
These values don't have value to the end-user, they're useful for facilitating the operation of the connection. Exposing them through just pollutes the schema and could confuse end-users.
There was a problem hiding this comment.
Alright, that makes sense. However the expand is now doing a bit too much, it would be nice to have these "supplements" in separate funcs within the main expand switch statement.
| options.AgentIP = existingOptions.AgentIP | ||
| options.AgentVersion = existingOptions.AgentVersion | ||
| options.AgentMode = existingOptions.AgentMode | ||
| options.ExtGroups = existingOptions.ExtGroups |
There was a problem hiding this comment.
Aren't these part of the "scopes" property already?
There was a problem hiding this comment.
No, these do not exist already.
There was a problem hiding this comment.
I am specifically referring to the 2 ext_* properties, see https://github.com/auth0/terraform-provider-auth0/blob/main/internal/auth0/connection/resource_test.go#L275-L276.
There was a problem hiding this comment.
Seems that erroneously the scope tag is missing from these https://github.com/auth0/go-auth0/blob/main/management/connection.go#L896, so we can keep them like this for now.
There was a problem hiding this comment.
Ah that's my bad I didn't realise these should be configured as scopes in the Go SDK
| @@ -13,7 +13,7 @@ import ( | |||
| "github.com/auth0/terraform-provider-auth0/internal/mutex" | |||
| ) | |||
|
|
|||
| const providerName = "Terraform-Provider-Auth0" | |||
| const providerName = "Terraform-Provider-Auth0" //#nosec G101 -- provider name not secret | |||
There was a problem hiding this comment.
Our CI's linting step started getting caught here
🔧 Changes
A notable behavior of the management API's PATCH
/api/v2/connections/{id}endpoint is that theoptionsproperty does not abide by normal PATCH conventions. Instead, the entire object will be replaced by the one provided in the PATCH payload.This behavior is problematic because there are several unconfigurable connection options for various connection types that need to be preserved for the connection to operate properly. This PR attempts to preserve several of these by reading the current connection options and "passing-through" several of them to the PATCH payload.
Granted, this is a short-term solution. Long-term, we'll need to devise a more holistic solution to avoid frequent, persistent updates to these pass-through properties.
📚 References
🔬 Testing
Passes existing acceptance tests. Also, if you observe the new recordings, you'll see that some of the "pass-through" properties are being included in the PATCH payload.
📝 Checklist