fix: stop team update from deleting externally-managed scoped user roles#235
Open
fv-nhastings wants to merge 1 commit into
Open
Conversation
The team resource's create/update reconciliation (updateUserRoles) treated itself as the sole owner of a team's scoped user roles: it deleted every scoped user role on the server that was absent from the team's own user_role block. When a team is managed without inline user_role blocks and its roles are managed by standalone octopusdeploy_scoped_user_role resources, any team update therefore deleted all of those roles, forcing 2+ applies to converge (or an endless add/remove cycle if the team drifts every apply). PR OctopusDeploy#105 fixed the read/refresh path (filterUserRolesByPreviousState) but the write path was left untouched. This applies the same ownership filter to removals: only roles previously managed by this team resource (from prior state) are eligible for deletion; externally-managed roles are left alone. Adds unit coverage (TestUpdateUserRolesRemovalOwnership) and an acceptance test (TestAccOctopusDeployTeamUpdateKeepsStandaloneScopedUserRole) that changes a team while a standalone scoped user role exists and asserts an empty follow-up plan. The pre-existing TestAccOctopusDeployTeamScopedUserRoleNoConflict never updated the team, so it did not exercise this path. Fixes OctopusDeploy#180. Follow-up to OctopusDeploy#100 / OctopusDeploy#105.
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.
What
Stops the
octopusdeploy_teamresource from deleting scoped user roles that are managed outside the team resource (i.e. via standaloneoctopusdeploy_scoped_user_roleresources) whenever the team is created or updated.Fixes #180. Follow-up to #100 / #105.
Why
updateUserRoles(called on every teamCreate/Update) reconciled the team's scoped user roles by:newUserRolesfrom the team's own inlineuser_roleblocks,newUserRoles.When a team is managed without inline
user_roleblocks (roles managed by standaloneoctopusdeploy_scoped_user_roleresources),newUserRolesis empty, so step 3 deletes every scoped user role attached to the team. Terraform then detects the standalone resources as missing on the next refresh and recreates them — requiring 2+ applies to converge, or looping indefinitely if the team drifts on every apply.PR #105 addressed the symptom on the read/refresh path (
filterUserRolesByPreviousStateinmapTeamResourceToState), but the write path inresource_team.go::updateUserRoleswas never given the equivalent ownership check, so the deletion persisted (re-reported in #180).How
Apply the same ownership filter to removals. Removal candidates are now drawn only from roles this team resource previously managed (derived from prior state via the existing
filterUserRolesByPreviousStatehelper), rather than from every role on the server:Updatenow readsreq.Stateand passes the prioruser_roleset intoupdateUserRoles.Createpasses a null set (nothing was previously managed, so nothing is removable — and a freshly created team has no pre-existing roles anyway).findRemovedScopedUserRolesis now fed the filtered "previously managed" set instead of all server roles.Add/edit behavior is unchanged; only the removal set is narrowed. Roles managed by inline
user_roleblocks continue to be added/removed/edited exactly as before.Tests
Unit —
TestUpdateUserRolesRemovalOwnershipcovering the removal-candidate selection:user_roleblocks → externally-managed roles are not selected for removal (the octopusdeploy_scoped_user_roles always getting removed when octopusdeploy_team is modified #180 case);user_roleteam that drops one role from config → only that previously-owned role is removed, while an externally-managed role on the same team is left untouched.Acceptance —
TestAccOctopusDeployTeamUpdateKeepsStandaloneScopedUserRole: creates a team with a standaloneoctopusdeploy_scoped_user_role, changes the team's description to force a team update, and asserts an empty follow-up plan (the role survives). The pre-existingTestAccOctopusDeployTeamScopedUserRoleNoConflictnever updated the team after adding the standalone role, so it did not exercise this path.Validated red→green against a live Octopus instance: the new acceptance test fails on the unpatched code (post-update refresh plan shows
octopusdeploy_scoped_user_role ... will be created,Plan: 1 to add) and passes with the fix. ExistingTestFilterUserRolesByPreviousStatecontinues to pass.gofmtclean;go vetclean.