Skip to content
This repository was archived by the owner on Mar 8, 2022. It is now read-only.

Remove auth0_client.custom_login_page_preview#379

Merged
yvovandoorn merged 1 commit intoalexkappa:masterfrom
kpurdon:kpurdon/remove-custom-login-page-preview
Apr 23, 2021
Merged

Remove auth0_client.custom_login_page_preview#379
yvovandoorn merged 1 commit intoalexkappa:masterfrom
kpurdon:kpurdon/remove-custom-login-page-preview

Conversation

@kpurdon
Copy link
Copy Markdown
Contributor

@kpurdon kpurdon commented Apr 22, 2021

Proposed Changes

  • Removes a legacy, information-only field custom_login_page_preview.

Fixes #360

Acceptance Test Output

kpurdon@syndio: ~/.../kpurdon/terraform-provider-auth0 make testacc TESTS=TestAccClient
==> Checking that code complies with gofmt requirements...
?   	github.com/alexkappa/terraform-provider-auth0	[no test files]
=== RUN   TestAccClientGrant
--- PASS: TestAccClientGrant (23.79s)
=== RUN   TestAccClient
--- PASS: TestAccClient (2.65s)
=== RUN   TestAccClientZeroValueCheck
--- PASS: TestAccClientZeroValueCheck (5.78s)
=== RUN   TestAccClientRotateSecret
--- PASS: TestAccClientRotateSecret (4.26s)
=== RUN   TestAccClientInitiateLoginUri
--- PASS: TestAccClientInitiateLoginUri (0.03s)
=== RUN   TestAccClientJwtScopes
--- PASS: TestAccClientJwtScopes (3.88s)
=== RUN   TestAccClientMobile
--- PASS: TestAccClientMobile (10.19s)
=== RUN   TestAccClientMobileValidationError
--- PASS: TestAccClientMobileValidationError (0.02s)
PASS
coverage: 20.7% of statements
ok  	github.com/alexkappa/terraform-provider-auth0/auth0	51.349s	coverage: 20.7% of statements
?   	github.com/alexkappa/terraform-provider-auth0/auth0/internal/debug	[no test files]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/alexkappa/terraform-provider-auth0/auth0/internal/random	0.564s	coverage: 0.0% of statements [no tests to run]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/alexkappa/terraform-provider-auth0/auth0/internal/validation	0.818s	coverage: 0.0% of statements [no tests to run]
?   	github.com/alexkappa/terraform-provider-auth0/version	[no test files]

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

@kpurdon
Copy link
Copy Markdown
Contributor Author

kpurdon commented Apr 22, 2021

@yvovandoorn @alexkappa this is ready for review, thanks!

@yvovandoorn
Copy link
Copy Markdown
Contributor

This looks good @kpurdon. Good call on removing it as I can see this could only cause problems.

@yvovandoorn yvovandoorn merged commit 95db4e8 into alexkappa:master Apr 23, 2021
@mars64
Copy link
Copy Markdown

mars64 commented Apr 26, 2021

Breaking the provider by removing this field seems less than ideal. I'd have preferred a deprecation warning here.

@yvovandoorn
Copy link
Copy Markdown
Contributor

Breaking the provider by removing this field seems less than ideal. I'd have preferred a deprecation warning here.

Thats fair, I'll update the CHANGELOG to be more explicit about removing this field.

@mars64
Copy link
Copy Markdown

mars64 commented Apr 28, 2021

Thats fair, I'll update the CHANGELOG to be more explicit about removing this field.

Thanks for the consideration!

Credit where due: I found what I needed in the existing changelog, it's just that it's called out as a BUG FIX, not a BREAKING CHANGE so I'd initially glossed over it. That's my bad, just getting used to the style of this project is all :)

I was thinking something more like using the terraform provider deprecation best practice: https://www.terraform.io/docs/extend/best-practices/deprecations.html

To be clear, I'm glad this field is removed 🎉 Thanks to all involved.

@kpurdon
Copy link
Copy Markdown
Contributor Author

kpurdon commented Apr 28, 2021

@yvovandoorn interestingly now I'm basically seeing the same thing w/ custom_login_page. Even after removing and disabling the page it's a permanent response requiring a state ignore. Should we apply the same (or improved) treatment to that field?

janpieper added a commit to janpieper/terraform-provider-auth0 that referenced this pull request Apr 30, 2021
@kpurdon kpurdon deleted the kpurdon/remove-custom-login-page-preview branch June 28, 2021 13:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permanent Diff - Auth0 Client - Custom Login Page Preview

3 participants