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

Make auth0_branding_theme fields optional #499

Merged
merged 4 commits into from
Mar 3, 2023
Merged

Make auth0_branding_theme fields optional #499

merged 4 commits into from
Mar 3, 2023

Conversation

nialdaly
Copy link
Contributor

Summary

This PR aims to close this issue.

@nialdaly nialdaly marked this pull request as ready for review February 18, 2023 13:26
@nialdaly nialdaly requested a review from a team as a code owner February 18, 2023 13:26
@nialdaly
Copy link
Contributor Author

nialdaly commented Feb 18, 2023

Hey @sergiught I might be running into a similar issue with the HTTP test recordings! When I try to recreate the recording using make test-acc-record FILTER="TestAccBrandingTheme", using a test Auth0 tenant (free subscription) I get the following:

=== RUN   TestAccBrandingTheme
    resource_theme_test.go:190: Step 1/2 error: Error running apply: exit status 1
        
        Error: failed to send the request: Post "https://dev-i0gs0cv5.us.auth0.com/api/v2/branding/themes": requested interaction not found
        
          with auth0_branding_theme.my_theme,
          on terraform_plugin_test.tf line 2, in resource "auth0_branding_theme" "my_theme":
           2: resource "auth0_branding_theme" "my_theme" {
        
--- FAIL: TestAccBrandingTheme (0.65s)
FAIL

Copy link
Contributor

@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.

Hey @nialdaly thanks for picking this one up! 💪🏻

The issue with the tests is that the old interactions weren't deleted before regenerating them. It's the first warning in the contribution section here https://github.com/auth0/terraform-provider-auth0/blob/main/CONTRIBUTING.md#adding-new-http-test-recordings. Unfortunately the make test-acc-record command doesn't do that yet (we were discussing internally whether we wanted to add that capability or not but decided to skip it for now as the FILTER is pretty much a regex and we could delete more files than desired).

After doing that would you mind adding a couple of new tests to ensure we can create a branding theme only with required properties? 🙏🏻

Don't hesitate to ping again if I can clarify further or help.

@nialdaly
Copy link
Contributor Author

nialdaly commented Feb 18, 2023

Thanks for getting back to me! @sergiught I deleted the old interaction and I'm getting this for the test failure:

=== RUN   TestAccBrandingTheme
    resource_theme_test.go:190: Step 1/2 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # auth0_branding_theme.my_theme will be updated in-place
          ~ resource "auth0_branding_theme" "my_theme" {
              + display_name = "Unnamed Theme"
                id           = "XGxYqNZiCg8kzjW4tBLL0JmOGxkYH42o"
        
        
              ~ colors {
                  + base_focus_color          = "#635dff"
                  + base_hover_color          = "#000000"
                    # (16 unchanged attributes hidden)
                }
        
        
        
                # (4 unchanged blocks hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccBrandingTheme (2.54s)

I'm not sure of the best way to resolve that?

I was also wondering, now that all of the required fields are actually optional, what would be your recommended way to test that?

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2023

Codecov Report

Base: 87.15% // Head: 86.96% // Decreases project coverage by -0.20% ⚠️

Coverage data is based on head (a1e18d5) compared to base (ce8dfc1).
Patch coverage: 90.75% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #499      +/-   ##
==========================================
- Coverage   87.15%   86.96%   -0.20%     
==========================================
  Files          62       64       +2     
  Lines        9631     9679      +48     
==========================================
+ Hits         8394     8417      +23     
- Misses        946      973      +27     
+ Partials      291      289       -2     
Impacted Files Coverage Δ
internal/provider/provider.go 59.59% <ø> (ø)
internal/auth0/email/flatten.go 80.82% <80.82%> (ø)
internal/auth0/email/expand.go 84.42% <84.42%> (ø)
internal/auth0/email/resource.go 93.05% <93.05%> (ø)
internal/auth0/branding/resource_theme.go 97.07% <100.00%> (+0.22%) ⬆️
internal/auth0/email/resource_template.go 80.79% <100.00%> (ø)
internal/provider/test_helpers.go 0.00% <0.00%> (-46.30%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@nialdaly nialdaly requested a review from sergiught February 20, 2023 13:36
docs/index.md Outdated
@@ -33,17 +33,14 @@ better alternative.
<!-- schema generated by tfplugindocs -->
## Schema

### Required
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be part of this PR. You probably just ran "make docs" and it did this automatically. The CI step that runs make docs keeps the domain in the required section but if you have a specific version of terraform and go locally it will move it to the optional block. I plan to fix this in the upcoming days, but for now let's discard this change please 🙏🏻

Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"buttons_style": {
Type: schema.TypeString,
ValidateFunc: validation.StringInSlice([]string{"pill", "rounded", "sharp"}, false),
Required: true,
Optional: true,
Default: "rounded",
Description: "Buttons style. Available options: `pill`, `rounded`, `sharp`.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we please update as well all the descriptions to mention what the default value will be 🙏🏻 ? e.g.

Suggested change
Description: "Buttons style. Available options: `pill`, `rounded`, `sharp`.",
Description: "Buttons style. Available options: `pill`, `rounded`, `sharp`. Defaults to `rounded`. ",

status: 204 No Content
code: 204
duration: 1ms
- id: 0
Copy link
Contributor

@sergiught sergiught Feb 21, 2023

Choose a reason for hiding this comment

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

Regarding your testing question, we could probably have another step that doesn't set any of the options and then checks that the defaults get applied. e.g.

const testAccBrandingThemeCreate = `
resource auth0_branding_theme my_theme {

}
`

// ... and then add another test step to check defaults in the actual test func

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! @sergiught I've made the changes as requested and tried to get it to a point where none of the options are set and the defaults are applied. There may be a neater way of doing it, more than happy for you to push changes on this branch. Thanks!

@sergiught
Copy link
Contributor

Hey @nialdaly 👋🏻 I can make some time today to help with the changes requested if needed and push on top of your branch. Just let me know:)

@nialdaly nialdaly requested a review from sergiught February 22, 2023 18:22
@sergiught
Copy link
Contributor

Heads up @nialdaly I'll make some time to take a look at this again and push some changes today.

Comment on lines +16 to +27
borders {}
colors {}
fonts {
title {}
subtitle {}
links {}
input_labels {}
buttons_text {}
body_text {}
}
page_background {}
widget {}
Copy link
Contributor

@sergiught sergiught Feb 24, 2023

Choose a reason for hiding this comment

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

There are a couple of problems and challenges to allow setting the minimal configuration for this resource to:

resource "auth0_branding_theme" "my_theme" { }
  1. If we set all the TypeList elements to Optional we can't ensure that the MinItems count is 1 because that is ignored when Optional is true.
  2. We can't use the Default property on TypeList or TypeSets. From the docs:
  // Default cannot be used if the Schema is directly an
	// implementation of an Elem field of another Schema, such as trying to
	// set a default value for a TypeList or TypeSet.

To work around this I've noticed that your attempt @nialdaly was to handle this within the expandBrandingTheme func, however that opens us up for serious drifts between the default values we set on the schema and potential erroneous changes of the default values within the expand func.

So to keep just the defaults on the schema as the only sources of truth we're requiring all the TypeList blocks to be set within the config at least as empty. The example highlighted here in the code above.

I'm not sure if you're an active user of this resource @nialdaly but what do you think of this? Tagging as well @TiltUp-Rad as the reporter of the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sergiught my initial approach was to require all of the blocks and then I went down the rabbit hole of trying to achieve the minimal configuration. I'm not an active user but I think the approach you suggest makes sense as the API does actually require the values!

@sergiught
Copy link
Contributor

Awesome contribution @nialdaly 🥳 thanks again!

@sergiught sergiught merged commit 8668fd7 into auth0:main Mar 3, 2023
@nialdaly nialdaly deleted the optional-auth0-branding-theme branch March 5, 2023 23:51
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