Fix pkgrepo.managed honouring clean_file when line already present (#68208)#69432
Open
dwoz wants to merge 2 commits into
Open
Fix pkgrepo.managed honouring clean_file when line already present (#68208)#69432dwoz wants to merge 2 commits into
dwoz wants to merge 2 commits into
Conversation
added 2 commits
June 14, 2026 23:00
pkgrepo.managed compared the desired repo definition against the result of pkg.get_repo and, when they matched, short-circuited with "already configured" — even when the caller passed clean_file: True. That left any unrelated stale entries in the managed file untouched, with no reported change. The user's bookworm-backports state that also still listed bullseye-backports is the canonical example. Skip the short-circuit comparison when clean_file is requested so the truncate + reconfigure path always runs in that case. The post-mod_repo change-diff loop still reports the change correctly. Fixes saltstack#68208
The first cut for saltstack#68208 unconditionally skipped the pre/sanitized-kwargs short-circuit when clean_file was set. That broke the idempotency contract that the test_adding_repo_file_signedby[Without(aptsources.sourceslist)] functional test exercises: after a successful first run the file already contains exactly the desired line, and a subsequent test=True run should report no changes -- but the unconditional skip caused the test=True path to emit a spurious "file" diff (the file kwarg was no longer popped), producing changes != {}. Narrow the condition: only force the clean + reconfigure path when the managed file currently holds any non-blank, non-comment line other than the desired one. The exact-match-only case still short-circuits to "already configured", so repeated runs stay idempotent. Adds a companion regression test (the all-desired-line file remains no-op) alongside the original saltstack#68208 regression test (a file with stale lines triggers truncate + mod_repo).
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
1 participant
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 does this PR do?
Makes
pkgrepo.managedactually empty the managed file and reconfigurethe repo when the user passes
clean_file: Trueand the desired repoline already happens to be present in the file alongside other lines.
What issues does this PR fix or reference?
Fixes #68208
Previous Behavior
If the file the state manages already contained the requested repo line
(plus unrelated stale lines the user wanted removed),
pkg.get_reporeturned the matching definition, the kwarg-vs-
precomparison fellthrough to the
elsebranch, and the state returnedwith no changes — silently skipping both the file truncation and the
re-write that
clean_file: Trueasks for. The user's/etc/apt/sources.list.d/backports.listlisting bothbullseye-backportsandbookworm-backportsis the canonical example:managing the bookworm line with
clean_file: Trueshould drop thebullseye line but didn't.
New Behavior
When
clean_file: Trueis set, the short-circuit "already configured"comparison is skipped entirely. The state proceeds to truncate the file,
call
pkg.mod_repoto re-write it, and reports the change via theexisting post-mod_repo diff loop.
Merge requirements satisfied?
"empty the file before configuring the defined repository", which
is now what actually happens)
changelog/68208.fixed.md)(
tests/pytests/unit/states/test_pkgrepo.py::test_managed_clean_file_with_matching_existing_repo_68208)Commits signed with GPG?
No (matches the base-branch convention)