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

refactor(product_enablement): make Read() logic consistent with other resource types #773

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Oct 18, 2023

In a prior PR we fixed a bunch of bugs with the design of the product_enablement nested resource block.

I noticed that we still had logic related to the old implementation.

For example, in the original implementation, we were always setting data into the state file when doing the TF 'read flow'. This caused problems with users seeing an unexpected diff because their config didn't contain something that now existed in the state file. Still part of the original implementation was a follow-up bug fix for that problem (i.e. we would only execute the 'read flow' if the user actually had product_enablement defined in their config).

As part of the recent PR rewrite for product_enablement, I noticed that we still had the conditional check that meant we wouldn't execute the 'read flow' unless the user had actually defined product_enablement in their config.

So this PR fixes that by removing those unnecessary requirements, and in the course of doing that I've fixed two other bugs that then surfaced because of that change...

  1. We now only set the name attribute if the user has product_enablement defined in their config. If we didn't do this, then we would A.) still be setting state data (albeit just the name attribute) which is something we were trying to avoid, and B.) if the user had allowed their service version to drift outside of TF, then the TF logic for handling that and bringing the versions back inline with what TF was tracking would cause a runtime panic because we would force the execution of the 'read flow' which would have tried to index (e.g. localState[0]) into an empty list.
  2. Now we're no longer setting anything into the state unless we're supposed to (see point 1.) we could end up in a situation where appending an empty map data type to a TypeSet causes a runtime panic. So we check for that first.

@Integralist Integralist added the enhancement New feature or request label Oct 18, 2023
@Integralist Integralist merged commit e275d7c into main Oct 18, 2023
7 checks passed
@Integralist Integralist deleted the integralist/refactor-prod-enablement branch October 18, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants