[1/2] DXCDT-426: Add new `auth0_connection_clients` resource #568

sergiught · 2023-05-08T14:09:41Z

🔧 Changes

In this PR we are adding a brand new auth0_connection_clients resource that managed the relationship between one connection and its respective enabled clients.

Warning
This new resource should not be used together with the auth0_connection_client resource, as they both operate on the same property of the connection, but one does so in bulk and the other is append only.

Note
In this PR we are not adding the logic to detect if both the 1:1 and 1:many resources for the connection are used within the same config. That will be a separate follow up through DXCDT-446. However that will only raise issues within the same config. It's still advised to not use both resources to manage the enabled clients of the same connection even if they are in separate terraform configurations.

📚 References

🔬 Testing

📝 Checklist

All new/changed/fixed functionality is covered by tests (or N/A)
I have added documentation for all new/changed functionality (or N/A)

sergiught · 2023-05-08T14:10:22Z

docs/resources/connection_client.md

-With this resource, you can manage enabled clients on a connection.
+With this resource, you can enable a single client on a connection.
+
+!> To prevent issues, avoid using this resource together with the `auth0_connection_clients` resource.


https://developer.hashicorp.com/terraform/registry/providers/docs#callouts Red warning.

sergiught · 2023-05-08T14:10:50Z

docs/resources/connection_client.md


 ## Example Usage

 ```terraform
 resource "auth0_connection" "my_conn" {
  name     = "My-Auth0-Connection"
  strategy = "auth0"
-  # Avoid using the enabled_clients = [...],


This is from the time we allowed managin the enabled_clients on the connection resource. Should not be here any more.

sergiught · 2023-05-08T14:11:18Z

internal/auth0/connection/resource_clients.go

+			},
+			"name": {
+				Type:        schema.TypeString,
+				Computed:    true,


The name and strategy are readonly.

sergiught · 2023-05-08T14:12:06Z

internal/auth0/connection/resource_clients.go

+func readConnectionClients(ctx context.Context, data *schema.ResourceData, meta interface{}) diag.Diagnostics {
+	api := meta.(*management.Management)
+
+	connection, err := api.Connection.Read(


We only need to lock the mutex if we're operating changes.

sergiught · 2023-05-08T14:12:42Z

templates/resources/connection_client.md.tmpl

+
+{{ .Description | trimspace }}
+
+!> To prevent issues, avoid using this resource together with the `auth0_connection_clients` resource.


We're overriding the default template for a resource so we can add the red warning for these resources.

codecov-commenter · 2023-05-08T14:28:34Z

Codecov Report

Patch coverage: 79.38% and project coverage change: -0.11 ⚠️

Comparison is base (c0e6308) 87.52% compared to head (4ecc7d4) 87.41%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
- Coverage   87.52%   87.41%   -0.11%     
==========================================
  Files          64       65       +1     
  Lines        9909    10039     +130     
==========================================
+ Hits         8673     8776     +103     
- Misses        936      958      +22     
- Partials      300      305       +5

Impacted Files	Coverage Δ
internal/provider/provider.go	`59.00% <ø> (ø)`
internal/auth0/connection/resource_clients.go	`79.23% <79.23%> (ø)`
internal/auth0/connection/resource_client.go	`76.27% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

willvedd

Overall solid and sensible contribution. A few ideas but I consider them non-blocking and could be implemented later.

willvedd · 2023-05-08T16:54:06Z

internal/auth0/connection/resource_clients.go

+
+	data.SetId(connection.GetID())
+
+	if len(connection.GetEnabledClients()) != 0 {


Optimization idea: Perhaps check the existing clients against the proposed clients. NBD if not implemented though.

Good idea, I'll improve the logic for this check in a follow up PR and I'll post the link here, so we can review before merging this one too.

Followed up in #570.

willvedd · 2023-05-08T16:54:53Z

internal/auth0/connection/resource_clients.go

+			diag.Diagnostic{
+				Severity: diag.Error,
+				Summary:  "Connection with non empty enabled clients",
+				Detail: "The connection already has enabled clients attached to it. " +


Considering we have the connection ID at this point, can we pre-populate the import command the user would need to execute?

I was trying to do that, but I couldn't find any way to get the name of the resource.

Given this resource

resource auth0_connection_clients some_custom_name { ... }

The import command would be: terraform import auth0_connection_clients.some_custom_name <conn_id>.

We could just print it anyway like that tho terraform import auth0_connection_clients.<your-resources-name> conn_123123 wdyt?

Followed up in #570.

willvedd · 2023-05-08T17:08:34Z

internal/auth0/connection/resource_clients_test.go

+	connection_id = auth0_connection.my_conn.id
+	enabled_clients = []
+}
+`


Idea: test the deletion of this resource by using a connection data source to check how many enabled clients there are.

Followed up in #570.

willvedd · 2023-05-08T17:10:51Z

internal/auth0/connection/resource_clients.go

+
+		return diag.Diagnostics{
+			diag.Diagnostic{
+				Severity: diag.Error,


Non-blocking idea: perhaps this could be a warning? While it's safe to be an error, it does strike me as being a bit heavy-handed. Not a big deal either way though.

If we improve the check above we could only throw the error when there's a risk of wiping out untracked client IDs on the enabled clients property of the connection.

Followed up in #570.

sergiught commented May 8, 2023

View reviewed changes

sergiught marked this pull request as ready for review May 8, 2023 14:12

sergiught requested a review from a team as a code owner May 8, 2023 14:12

Add new auth0_connection_clients resource

4ecc7d4

sergiught force-pushed the feature/DXCDT-426-connection-clients branch from 6cd4510 to 4ecc7d4 Compare May 8, 2023 14:19

willvedd approved these changes May 8, 2023

View reviewed changes

sergiught mentioned this pull request May 9, 2023

[2/2] DXCDT-426: Improve how we guard against erasing unwanted enabled clients #570

Merged

2 tasks

sergiught changed the title ~~DXCDT-426: Add new auth0_connection_clients resource~~ [1/2] DXCDT-426: Add new auth0_connection_clients resource May 9, 2023

sergiught requested a review from willvedd May 9, 2023 10:15

sergiught merged commit 7fc819b into main May 9, 2023

sergiught deleted the feature/DXCDT-426-connection-clients branch May 9, 2023 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/2] DXCDT-426: Add new `auth0_connection_clients` resource #568

[1/2] DXCDT-426: Add new `auth0_connection_clients` resource #568

sergiught commented May 8, 2023

sergiught May 8, 2023

sergiught May 8, 2023

sergiught May 8, 2023

sergiught May 8, 2023

sergiught May 8, 2023

codecov-commenter commented May 8, 2023

willvedd left a comment

willvedd May 8, 2023

sergiught May 8, 2023

sergiught May 9, 2023

willvedd May 8, 2023

sergiught May 8, 2023

sergiught May 9, 2023

willvedd May 8, 2023

sergiught May 9, 2023

willvedd May 8, 2023

sergiught May 8, 2023

sergiught May 9, 2023


		{{ .Description \| trimspace }}

		!> To prevent issues, avoid using this resource together with the `auth0_connection_clients` resource.


		data.SetId(connection.GetID())

		if len(connection.GetEnabledClients()) != 0 {

[1/2] DXCDT-426: Add new auth0_connection_clients resource #568

[1/2] DXCDT-426: Add new auth0_connection_clients resource #568

Conversation

sergiught commented May 8, 2023

🔧 Changes

📚 References

🔬 Testing

📝 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 8, 2023

Codecov Report

willvedd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[1/2] DXCDT-426: Add new `auth0_connection_clients` resource #568

[1/2] DXCDT-426: Add new `auth0_connection_clients` resource #568