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

DXCDT-482: Fix deleting the universal login template within auth0_branding #695

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

sergiught
Copy link
Contributor

@sergiught sergiught commented Jul 7, 2023

🔧 Changes

As raised within #659, removing the universal login block does not trigger a deletion of the template. This PR fixes that and further improves the auth0_branding resource, more info within comments.

📚 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)

Computed: true,
Description: "The body of login pages.",
Type: schema.TypeString,
Required: 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.

Marking this as required to prevent instances where the font block is present with no attributes assigned.

@@ -66,6 +73,7 @@ func NewResource() *schema.Resource {
"font": {
Type: schema.TypeList,
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.

Marking this now as computed in order to not require any custom logic within the read func that only reads the font details if this is present within the config as it goes against best practices.

@@ -32,6 +38,7 @@ func NewResource() *schema.Resource {
"colors": {
Type: schema.TypeList,
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.

Marking this now as computed in order to not require any custom logic within the read func that only reads the colors details if this is present within the config as it goes against best practices.

)
if _, ok := d.GetOk("colors"); ok {
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 a very weird custom logic we had within the read func that was only assigning the API data if the config blocks were present.


if err := checkForCustomDomains(api); err == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're only fetching template data if there's a custom domain set.

Comment on lines +152 to +163
oldUL, newUL := d.GetChange("universal_login")
oldUniversalLogin := oldUL.([]interface{})
newUniversalLogin := newUL.([]interface{})

// This indicates that a removal of the block happened, and we need to delete the template.
if len(newUniversalLogin) == 0 && len(oldUniversalLogin) != 0 {
if err := api.Branding.DeleteUniversalLogin(); err != nil {
return diag.FromErr(err)
}

return readBranding(ctx, d, m)
}
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 is the part that actually fixes the GitHub issue around the deletion of the template.

"because no custom domains are available on the tenant.",
AttributePath: cty.Path{cty.GetAttrStep{Name: "universal_login"}},
},
if err == errNoCustomDomain {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The delete func was also improved to directly allow the deletion of the resource if there are no custom domains set.

Comment on lines -270 to -272
if universalLogin == nil {
return nil, nil
}
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 unnecessary.

@sergiught sergiught marked this pull request as ready for review July 7, 2023 17:25
@sergiught sergiught requested a review from a team as a code owner July 7, 2023 17:25
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.

Nice 👍

@sergiught sergiught merged commit 0e8f327 into main Jul 10, 2023
4 checks passed
@sergiught sergiught deleted the patch/DXCDT-482-ul-delete branch July 10, 2023 19:32
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.

3 participants