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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to set a pingone_branding_theme as the active theme for an environment #310

Closed
patrickcping opened this issue Mar 23, 2023 · 5 comments 路 Fixed by #375
Closed

Ability to set a pingone_branding_theme as the active theme for an environment #310

patrickcping opened this issue Mar 23, 2023 · 5 comments 路 Fixed by #375
Assignees
Labels
service/base PingOne Platform size/medium Medium size change (e.g. enhancing the logic of an existing resource or datasource) type/enhancement New feature or request
Milestone

Comments

@patrickcping
Copy link
Collaborator

Community Note

  • Please vote on this issue by adding a 馃憤 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

When configuring a theme with pingone_branding_theme there needs to be a way to set it as the active theme in the environment. It is used for the sign-on policies, and also as a fallback for DaVinci errors.

New or Affected Resource(s)

  • pingone_branding_theme

Potential Terraform Configuration

# Copy-paste your PingOne related Terraform configurations here - for large Terraform configs,
# please use a service like Dropbox and share a link to the ZIP file. For
# security, you can also encrypt the files using our GPG public key.

# Remember to replace any account/customer sensitive information in the configuration before submitting the issue

References

@patrickcping patrickcping added type/enhancement New feature or request service/base PingOne Platform labels Mar 23, 2023
@patrickcping patrickcping added this to the v0.12.0 milestone Mar 23, 2023
@patrickcping patrickcping modified the milestones: v0.12.0, v0.13.0 Apr 14, 2023
@patrickcping patrickcping modified the milestones: v0.13.0, v0.14.0 Apr 24, 2023
@patrickcping patrickcping modified the milestones: v0.14.0, v0.15.0 May 11, 2023
@patrickcping
Copy link
Collaborator Author

The default attribute on the pingone_branding_theme is currently a computed/read-only value although it is mutable on the API itself which prevents setting a theme as default/active.

It's set as read-only because API validation restrictions mean conflicts arise when updating the value between true/false after initial creation, and removing a theme set as active/default.

  • The apply phase can end up in a locked situation where it is waiting for another theme to be set as the default before setting true=>false
  • There is not a clean way to ensure an alternative default is configured on the destroy of a theme where default==true

The proposed way forward is to create a new, additional resource pingone_branding_theme_default and continue to restrict switching of the default parameter on pingone_branding_theme.

@dbryar
Copy link

dbryar commented May 15, 2023

@patrickcping there is a clean way to do this, without creating the hassle of an orphan setting or resulting in locked loops.

Mark the default value options as true | undefined

as per #357 the flag is only passed "if default === true"

If multiple themes are marked as true, either trigger a failure message (uniqueness constraint) or set the most recent called as default (undesirable, but not broken)

If the issue is in the TF validation, there is always the fallback default theme Ping Default that can be either referenced, or even recreated if it has been deleted?

@patrickcping patrickcping self-assigned this May 22, 2023
@patrickcping patrickcping added the size/medium Medium size change (e.g. enhancing the logic of an existing resource or datasource) label May 22, 2023
@patrickcping
Copy link
Collaborator Author

Thanks for that, a few considerations:

Mark the default value options as true | undefined

From an API perspective, the value cannot be left undefined - it will default to false when computed back into state, so we're not able to "unset" to "reset" because of the lock situation.

If multiple themes are marked as true, either trigger a failure message (uniqueness constraint)

Terraform's resources (even if they're the same type) aren't aware of each other as they're being applied. A uniqueness error would prevent a locked loop but I'm not sure we can get there cleanly

set the most recent called as default (undesirable, but not broken)

This is how the API currently works and will work reasonably well until changing the default from one to another. This will lead us to the lock situation or a state management problem. The state management problem is where "Theme 2" takes the default from "Theme 1", but "Theme 1" is expecting default == true in it's state - it'll result in TF thinking there is unintended drift and either want to reset it back or show inconsistent state warnings after apply.

If the issue is in the TF validation, there is always the fallback default theme Ping Default that can be either referenced, or even recreated if it has been deleted?

This is the most workable, but we have an objective to put all configuration in the platform under some form of "drift control". Where we have to fallback to the default theme on delete it will need to be TF unmanaged. This is where the idea of the pingone_branding_theme_default comes from, to pull that item of config under TF control and be able to detect it's drift

@dbryar
Copy link

dbryar commented Jan 29, 2024

Is there any update to the issues surrounding the inability to set defaults?

@patrickcping
Copy link
Collaborator Author

I've responded more generally on issue #463 , but the good news is that the branding themes are one of the few where we don't encounter conflicts between the API design and Terraform's implementation.

You're able to set the default branding theme with the pingone_branding_theme_default resource (docs link)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service/base PingOne Platform size/medium Medium size change (e.g. enhancing the logic of an existing resource or datasource) type/enhancement New feature or request
Projects
None yet
2 participants