fix: Set push group destroy default on import - #2905
Open
exitcode0 wants to merge 1 commit into
Open
Conversation
delete_target_group_on_destroy is declared Optional/Computed with a default of true, but nothing in the read path ever writes it. ImportState sets only app_id and id, and mapPushGroupResourceToState (shared by Create, Read and Update) does not touch the attribute, so after terraform import the value is null in state. That is visible as a spurious in-place diff on the next plan, and it also changes destroy behaviour: Delete passes the state value to the API as DeleteTargetGroup(state.DeleteTargetGroupOnDestroy.ValueBool()), and ValueBool() on a null types.Bool returns false. An imported mapping that has not been applied destroys with deleteTargetGroup=false, while the same configuration created by Terraform destroys with true. The attribute has no counterpart in the API response (it exists only as a query parameter on the delete call), so it cannot be read back from Okta. Import now seeds it with the documented default instead, and the shared state mapper falls back to that default only when the incoming value is null, so state written by an import on an earlier provider version heals on refresh while an explicit false is preserved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2827
delete_target_group_on_destroyis declaredOptional+ComputedwithDefault: booldefault.StaticBool(true), but nothing in the read path ever writes it.ImportStatesets onlyapp_idandid, andmapPushGroupResourceToState(shared byCreate,ReadandUpdate) does not touch the attribute. Afterterraform importthe value is therefore null in state.That has two effects:
Deletepasses the state value straight to the API asDeleteTargetGroup(state.DeleteTargetGroupOnDestroy.ValueBool()), andValueBool()on a nulltypes.Boolreturnsfalse. An imported mapping that has not been through an apply destroys withdeleteTargetGroup=false, while the same configuration created by Terraform destroys withdeleteTargetGroup=true. Whether the downstream target group is removed depends on how the resource entered state rather than on the configuration.The attribute has no counterpart in the API response, it only exists as a query parameter on the delete call, so it cannot be read back from Okta. The fix is to make imported state carry the documented default instead of null.
Changes
ImportStatenow seedsdelete_target_group_on_destroywithtruealongsideapp_idandid, so an imported mapping starts out in the same shape as a created one.mapPushGroupResourceToStatefalls back totrueonly when the incoming value is null. On create and update the value comes from the plan, and on read from prior state, and in all of those cases it is already known, so an operator's explicitfalseis never overwritten. The guard exists so that state written by an import on an earlier provider version heals on the next refresh, including the refresh that precedes a destroy, rather than only after an intervening apply.TestAccResourceOktaPushGroup_import, which applies the existing fixture and then imports the mapping withImportStateVerify.delete_target_group_on_destroyis seeded with the default at import, and thattarget_group_namestays unset.Doing this in both places is deliberate.
ImportStateis where the import-specific default belongs and makes the intent obvious at the point of import; the null guard in the shared mapper is what covers state that predates this change.target_group_nameis intentionally left alone. It is a genuine import gap, and the note above is only documentation, but it is not fixable here:GroupPushMappingin the v6 SDK has notargetGroupNamefield, so there is nothing to read back. Deriving it from the target group's current name would be a guess, and the attribute isRequiresReplace, so guessing wrong would propose destroying a live mapping. That seems worth a separate discussion rather than folding into this fix.No 404 handling is included here, since #2853 and #2892 already cover that in this file.
Testing
gofmt -lon the changed files,go build ./...,go vet ./okta/...— clean.golangci-lint fmtcould not run locally: the installed binary is built with go1.25 and the module targets go1.26.2.make test(unit tests) — pass, no failures.OKTA_VCR_TF_ACC=play:TestAccResourceOktaPushGroup_crudpasses, both before and after the change.TestAccResourceOktaPushGroup_adand the new import test are skipped in play mode, as they have no cassettes.make test-play-vcr-acc. It was verified against the existingTestAccResourceOktaPushGroup_crudcassette by temporarily inserting the same import step into that test:masterthe step fails withImportStateVerify attributes not equivalent, the difference being the missingdelete_target_group_on_destroy = trueTestAccResourceOktaPushGroup_crudinstead if a maintainer can re-recordTestAccResourceOktaPushGroup_crud/classic-00.yaml, or to drop the new test if you would rather recordTestAccResourceOktaPushGroup_importseparately.