DXCDT-94 Stop ignoring errors when setting resource data within the client #94
Conversation
At this point we will be getting the following errors from the tests as we now stop ignoring errors:
Error: 2 errors occurred:
* addons: '': source data must be an array or slice, got map
* mobile: '': source data must be an array or slice, got map
| @@ -19,7 +19,6 @@ func newDataClient() *schema.Resource { | |||
| func newClientSchema() map[string]*schema.Schema { | |||
| clientSchema := datasourceSchemaFromResourceSchema(newClient().Schema) | |||
| delete(clientSchema, "client_secret_rotation_trigger") | |||
| delete(clientSchema, "client_secret") | |||
There was a problem hiding this comment.
Because we are reusing the same schema as the client resource, if we delete the client_secret from here, when reading an entire client we will be getting an error when setting client_secret within the readClient() func as it's not present in the schema any more.
This should have never been deleted here because of this reason.
There was a problem hiding this comment.
We had originally agreed to omit client_secret from the data source. Unless I'm missing it down below, this change appears to repeal that decision (confirmed by the change in the test). I see that this method of deletion is problematic but it would be wise to scrub the client secret some other way.
There was a problem hiding this comment.
That's indeed true, however at the time we weren't aware that this would lead to an unsurfaced error. Considering the resource already has the client_secret and this is something that the API simply returns, if not using the data_source we always have access to it through the resource. Having the client_secret will also help a lot with passing that information to other providers / resources for configuration purposes.
| @@ -34,7 +34,10 @@ func TestAccDataClientByName(t *testing.T) { | |||
| PreventPostDestroyRefresh: true, | |||
| Steps: []resource.TestStep{ | |||
| { | |||
| Config: random.Template(testAccClientConfig, rand), // must initialize resource before reading with data source | |||
| Config: random.Template(testAccClientConfig, rand), | |||
| Check: resource.ComposeTestCheckFunc( | |||
There was a problem hiding this comment.
After initializing the resource we just wanna make extra sure the client got created correctly so we can fetch it within the client data source below.
| @@ -62,6 +64,9 @@ func TestAccDataClientById(t *testing.T) { | |||
| Steps: []resource.TestStep{ | |||
| { | |||
| Config: random.Template(testAccClientConfig, rand), | |||
| Check: resource.ComposeTestCheckFunc( | |||
| resource.TestCheckResourceAttr("auth0_client.my_client", "name", fmt.Sprintf("Acceptance Test - %v", rand)), | |||
There was a problem hiding this comment.
After initializing the resource we just wanna make extra sure the client got created correctly so we can fetch it within the client data source below.
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "callback": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| }, | ||
| "slo_enabled": { | ||
| Type: schema.TypeBool, | ||
| Optional: true, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
In order to not need to introduce a breaking change to our users we need to remove the definition of each element of the schema.TypeMap as we're not allowed to have different types. This now follows the same strategy as for the other addon types. Like this we can think of deprecating the other ones in a subsequent PR.
Check the note on the TypeMap from the official docs: https://www.terraform.io/plugin/sdkv2/schemas/schema-types#typemap.
| d.Set("mobile", c.Mobile) | ||
| d.Set("initiate_login_uri", c.InitiateLoginURI) | ||
| d.Set("signing_keys", c.SigningKeys) | ||
| result := multierror.Append( |
There was a problem hiding this comment.
We stop ignoring all the potential errors from the Set func using the multierror pkg. This actually surfaces issues with our client resource that we need to fix. Ultimately fixing them also fixes 3 open issues we have within our github that are referenced in the description.
|
|
||
| func mapToState(input map[string]interface{}) map[string]interface{} { |
There was a problem hiding this comment.
In order to prevent us from introducing a breaking change we need to ensure that the TypeMap attribute always contains strings when in the state but actually use correct types when sending it to the Auth0 management API. We added a mapToState that will convert types like integers, bools, float to strings so all TypeMap key values are strings.
| if customSocial == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
We return early in case customSocial is nil. This decreases the level of indentation within this func.
| if refreshToken == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
We return early in case refreshToken is nil. This decreases the level of indentation within this func.
| return []interface{}{m} | ||
| } | ||
|
|
||
| func flattenClientAddons(addons map[string]interface{}) []interface{} { |
There was a problem hiding this comment.
To properly fix the surfaced error we needed to add a custom flatten of the client addons.
| return []interface{}{m} | ||
| } | ||
|
|
||
| func flattenClientMobile(mobile map[string]interface{}) []interface{} { |
There was a problem hiding this comment.
To properly fix the surfaced error we needed to add a custom flatten of the client mobile.
sergiught
changed the title
sergiught
requested review from
willvedd
and removed request for
a team and
willvedd
Description
This PR stops ignoring errors when setting resource data within the client.
This fixes:
Checklist
Note: Checklist required to be completed before a PR is considered to be reviewable.
Auth0 Code of Conduct
Auth0 General Contribution Guidelines
Changes include test coverage?
Does the description provide the correct amount of context?
Have you updated the documentation?
Is this code ready for production?