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

Add new auth0_organization_connection resource #253

Merged
merged 4 commits into from
Jul 22, 2022

Conversation

sergiught
Copy link
Contributor

@sergiught sergiught commented Jul 20, 2022

Description

Splitting the connections out of the organization resource into its own resource, however we keep the old connection sets on the organization until folks have enough time to migrate over. This is done to simplify the internal logic and improve DX.

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?

  • Yes
  • Not needed

Does the description provide the correct amount of context?

  • Yes, the description provides enough context for the reviewer to understand what these changes accomplish

Have you updated the documentation?

  • Yes, I've updated the appropriate docs
  • Not needed

Is this code ready for production?

  • Yes, all code changes are intentional and no debugging calls are left over

@sergiught sergiught self-assigned this Jul 20, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2022

Codecov Report

Merging #253 (89b13b5) into main (cad77de) will increase coverage by 0.03%.
The diff coverage is 85.43%.

@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
+ Coverage   83.33%   83.36%   +0.03%     
==========================================
  Files          37       38       +1     
  Lines        6985     7088     +103     
==========================================
+ Hits         5821     5909      +88     
- Misses        924      935      +11     
- Partials      240      244       +4     
Impacted Files Coverage Δ
auth0/provider.go 54.66% <ø> (ø)
auth0/resource_auth0_organization_connection.go 84.84% <84.84%> (ø)
auth0/resource_auth0_organization.go 80.48% <100.00%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cad77de...89b13b5. Read the comment docs.

Copy link
Contributor Author

@sergiught sergiught left a comment

Choose a reason for hiding this comment

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

Adding a few comments to aid with reviewing.

@@ -61,6 +61,9 @@ func newOrganization() *schema.Resource {
"connections": {
Type: schema.TypeSet,
Optional: true,
Computed: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly we need to mark this now as computed, otherwise if we use the newly introduced auth0_organization_connection to add a new connection, when terraform performs a refresh the auth0_organization resource will see that a new connection was added "out-of-bound" and it will try to remove it, but the new resource will try to add it again, causing an infinite loop.

We're marking this however as well as deprecated and it will get removed in the future, once everyone ports over to the new way of adding and removing connections on orgs, through the new resource.

Metadata: &metadata,
}

if d.HasChange("metadata") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While implementing the new resource and adding minimal tests for it, it was spotted that we need a HasChange over here when we first create the org, otherwise if we don't have the metadata the first time we create the org it will get sent as "null" and the API doesn't accept that.

Comment on lines +43 to +54
"name": {
Type: schema.TypeString,
Computed: true,
Description: "The name of the enabled connection.",
},
"strategy": {
Type: schema.TypeString,
Computed: true,
Description: "The strategy of the enabled connection.",
},
},
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proper example of readonly properties that might be useful to have.

@@ -0,0 +1,45 @@
---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was generated with the make docs-gen RESOURCE=organization_connection command.

@sergiught sergiught marked this pull request as ready for review July 20, 2022 10:30
@sergiught sergiught requested a review from a team as a code owner July 20, 2022 10:30
@sergiught sergiught marked this pull request as draft July 20, 2022 14:16
@sergiught sergiught force-pushed the feature/separate-org-conn-in-own-resource branch from ca20288 to 81c6e8e Compare July 20, 2022 15:31
@sergiught sergiught marked this pull request as ready for review July 20, 2022 15:31
@sergiught sergiught requested a review from willvedd July 21, 2022 07:13
@@ -61,6 +61,9 @@ func newOrganization() *schema.Resource {
"connections": {
Type: schema.TypeSet,
Optional: true,
Computed: true,
Deprecated: "This attribute will be removed in the future. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Deprecated: "This attribute will be removed in the future. " +
Deprecated: "Management of organizations through this property has been deprecated in favor of the `auth0_organization_connection` resource and will be deleted in future versions. It is advised to migrate all managed organization connections to the new resource type.",

Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

👍

@sergiught sergiught force-pushed the feature/separate-org-conn-in-own-resource branch from 0c9c8a3 to 89b13b5 Compare July 22, 2022 15:12
@sergiught sergiught merged commit 8b8aecf into main Jul 22, 2022
@sergiught sergiught deleted the feature/separate-org-conn-in-own-resource branch July 22, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants